This week I spent some time skimming through Refactoring: Ruby Edition and also put some more refactoring into Redmine. The book, unfortunately, is just a rehash of Fowler's original but it does have some nuggets of information. The continued refactoring of Redmine was both interesting and intriguing. I had a few tough decisions to make but I think that, in the end, the factoring of the code was much better than the original.

Refactoring: Ruby Edition

Jay Fields released this book back in the fall and I was really excited to grab it. It was a book specifically about one of the most interesting topics (refactoring) and in one of the most interesting languages (Ruby). I was really excited.

Unfortunately, I found that I've already read most of this book. All of the examples are, of course, in Ruby instead of Java but the examples are almost exactly the same. The catalog has, for the most part, the exact same content (I'll get to some of the differences in a second). Despite all of this, though, it is still a great reference and a great read if you haven't read Fowler's book already.

The first chapter I had already read and practiced. I did this in Chicago at 8th Light with Doug Bradbury. We went step by step through the chapter and recreated the chapter in code. It was fun.

The chapter on testing was significantly different; Jay talked about Test::Unit (and, briefly, RSpec) instead of JUnit. He gave a good overview and stuck to the spirit of Refactoring.

The catalog had a few differences that I should point out (also, I've only read half of the catalog, more next week on the other half). Specifically, Jay expands a few chapters (like Extract Method) to include several Rubyisms. This is nice because algorithms to refactor Java do not always translate completely to other languages (like Ruby).

Jay also introduced a new pattern that is very Rubyist but also very powerful.

Refactoring Redmine, Part 2

This week I'm revisiting Redmine. I really like working with this project because it

  • Has a lot (and I mean a lot!) of tests. This makes my life easier.
  • Has well factored code. This makes me really think about the refactorings. In situations such as the Android function, I don't need to think about the refactorings because the code is so bad that almost anything I do is an improvement.
  • Is written in Ruby and Rails. I just like these two technologies.

So, once again, I set off on a random controller and attempted to refactor. I selected the Project Controller and refactored the first method I saw (ProjectController#add).

Step 1

http://github.com/hammerdr/redmine/commit/3421454a776b838ade1049194469b39585250f19

Extract Method. Nothing special about this one.

Step 2

http://github.com/hammerdr/redmine/commit/efb6bcca1cca4a326fb859cb327f7abd020f1301

Extract method and add guard. This is my really cool Ruby refactor that I love to use. Seems to improve the code a lot.

Step 3

http://github.com/hammerdr/redmine/commit/09d75cb37601ef6b1433a179f7635f402abef6dc

Renaming to make the code more clear.

Step 4

http://github.com/hammerdr/redmine/commit/cc46fc59f81a3809e23943b782371f0dc6895444

Rename Method to follow Ruby convention.

Step 5

http://github.com/hammerdr/redmine/commit/6e348b0f9ec5c2538942f2c1083629d730504db1

Move Method. The comments already stated that they wanted to do this. I agreed. I moved the method to the Project class. I was faced with a tough decision here, though. I could have NOT passed in the parent_id because the params object is accessible in the Model, as well. However, I looked at the method and accessing the params object is not a key element to the method, but is instead just a way to access a parent_id. So, I passed it in as a parameter.

Step 6

http://github.com/hammerdr/redmine/commit/02e2490bdf9bb370d48a1225838448175e6eabc5

Move Method Block. I moved this code outside of the extracted method because it wasn't really doing what the method said it was doing.

Step 7

http://github.com/hammerdr/redmine/commit/39119d1f394009290548f5293ba24d4801a0cc1d

Extract Method. Nothing really special here.

Step 8

http://github.com/hammerdr/redmine/commit/45ace4ddcea8d826212f831ca77b9fff23279dd7

Using guards instead of an if block. If you look at this and think that this wasn't positive, just wait. I end up agreeing with you.

Step 9

http://github.com/hammerdr/redmine/commit/e22895a195947c07a3991d27979de3fdb35404fa

Extracting assignment to caller. I did this so that I could try something. I end up regressing some of this. I don't think that this was really positive or negative.

Step 10

http://github.com/hammerdr/redmine/commit/2532f6c728fb4948d2d094c6a1023bc245e7d7f7

Using a guard instead of an embedded if block.

Step 11

http://github.com/hammerdr/redmine/commit/697a49392eb7de4ea2c8a2abfc570b06b1455585

Reverting my guards and if block changes. The three statements that had the if block just looked ugly. I don't like the if block (too much nesting in this method) but we're still working.

Step 12

http://github.com/hammerdr/redmine/commit/cc6cbfa7e75fcbbe85d46d3d1af1d7bc2fab6994

Reverting the pull out here. Where I wanted to go wasn't going to happen so I reverted part of that change.

Step 13

http://github.com/hammerdr/redmine/commit/7ddcdc7facaca8fd96be78c86c9334716ba2f32a

Getting rid of that nasty if/else block. I don't like the "and return" here.

Step 14

http://github.com/hammerdr/redmine/commit/0088fb804e896f037088b2a2240280d3c51ddc89

Got rid of that nasty "and return" by extract method and add guard. Didn't know exactly what to call the method, yet.

Step 15

http://github.com/hammerdr/redmine/commit/c6a690147cb97c1c66f3321b1822c95a0144ffdd

Rename method.

Step 16

http://github.com/hammerdr/redmine/commit/48ffc423b75e8a1de46dac37dd6fbc7ba3c8a9fe

Encapsulation.