Wednesday, January 14, 2009

Refactoring as a learning exercise

Over the last several months I have been trying to add a new tool to my tool belt by learning Ruby, Ruby on Rails and RSpec. After completing some tutorials and other basic programming tasks I was looking for something else to further develop my skills. Not having any great ideas for new applications I turned to our SVN repository and the code base for an application that I was functionally familiar with. My goal was to look through the code base, understand what was going on and see if there was anything that I could refactor. I figured this would be a good way to learn from real world code. This code base is for a new, greenfield project and was developed with good RSpec coverage so it was a good candidate because the code was likely in good condition and it would really challenge me by stretching my Ruby and Rails skills. As I dug around I found some things that were of interest and possible candidates for refactoring:

  1. In trying to understand what one area of the code was doing I looked first to the RSpec tests. After reading the tests and then the code the tests were testing I realized that the tests were created at a higher level and as a result it was somewhat hard to infer the behavior of the code under test
  2. In that same area of the code several Model classes had quite a bit of hand written SQL code. Portions of the SQL was dynamically generated and as a result it was somewhat difficult to understand what was actually going on. I knew functionally what this area of the code was supposed to be doing but it was not totally clear from reading the code what it was actually doing
This was the opportunity I was looking for...could I figure out what was going on, refactor the code to better leverage ActiveRecord without having to write custom SQL and get the proper results at the end? Having a goal and learning opportunity in place here is what I did:

I started by writing my own RSpec tests at a lower level of detail. The existing tests tested higher level methods that called several lower level methods, did some calculations and returned a result. The new tests I wrote started by testing the lower level methods individually. This had two benefits: (1) I could get a better understanding of what each of the lower level methods were doing and (2) I could refactor each method individually and have tests that would ensure I was not breaking anything at the lowest level of detail. This was by far the slowest part of the exercise because I needed to work through much of the handwritten SQL to be able to understand what was expected from these methods so that I could write the tests. After several hours of walking through the code and testing out SQL's against the development database I had what I thought was a decent handle on what was going on so I created my first test and verified that I was able to get it to pass with the existing code. My next step was to then figure out how I could replace the hand written SQL with ActiveRecord API calls and retain the same functionality. The first place I started was with the ActiveRecord find_by Dynamic attribute-based finders. The first challenge I faced was that the handwritten SQL's had several joins that I needed to deal with. To be able to deal with these joins I added some associations to the appropriate model classes and began to dive into getting the right :group, :include and :conditions options to return what I needed. This was an amazing learning exercise in Rails and the power it possesses. Finally, after several more hours of trying different things on IRB I got something that seemed like it could work. During my research however, I bumped into the ActiveRecord Calculations module. Considering that the main goal of the SQL's I was trying to refactor was to sum column data I thought perhaps the sum method would be an even better option. After several passes through IRB with the sum method I found what I was looking for and lucky me, my first test passed. From this point on it was more of the same with 5 or 6 additional methods until I got to a point where all of the hadwritten SQL was replaced by ActiveRecord API calls and I had a set of RSpecs that tested each method individually, including the higher level method that calls all of the lower level methods. Having made it this far and feeling pretty good about the results, I identified some next steps to keep the learning going:
  1. complete the same exercise for the remaining model classes where similar hand written SQLs exist
  2. look for areas where I can extract duplicate code across all of those models into single, re-usable methods or classes
  3. work on refactoring the RSpecs to improve their performance. Currently they feel a bit slow due to, what I suspect, are the many database interactions that are happening
So for those of you that have actually read this far down, here is what I learned:
  1. Refactoring is a great way to learn about a programming language. If you are new to a language try finding some existing code, write some tests for it to get an understanding of what it is doing and then refactor the code while getting the tests to pass.
  2. Using TDD or BDD makes learning about a programming language easier
  3. ActiveRecord is a powerful tool that gives you a great way to interact with a database without having to write SQL

Labels: , , ,

2 Comments:

At January 14, 2009 at 12:31 PM , Blogger Alex Rothenberg said...

This comment has been removed by the author.

 
At January 14, 2009 at 12:34 PM , Blogger Alex Rothenberg said...

Mark,

Did you commit your changes back into source control? If you made it better let's not leave it in worse shape than it could be :)

 

Post a Comment

Subscribe to Post Comments [Atom]

<< Home