Recently I had the opportunity to review a ruby on rails code. The project aims to create an online market place for experts, with a similar model to some other freelancing/outsourcing sites. Here is how and what I reviewed:
I had a read-only access to the code and no download was allowed locally. So, I didn't get to run the code, tests or any tool to auto-generate some reports on the code. Also, it was supposed to be a high level review and I was supposed to take 5 hours to take a look into the code and write a report on the various aspects of it. In the end, it took me 6 hours and I came up with a 14 page report. I liked this job for a few reasons:
Also, its a good idea to know the Ruby language fluently to make use of the power of the language. A good learning resource comes with ruby installation named Programming Ruby, also available in the internet at http://ruby-doc.org
As a continuous learning tool, I also suggest keeping an eye on http://railscasts.com and http://stackoverflow.com/questions/tagged/ruby-on-rails
I had a read-only access to the code and no download was allowed locally. So, I didn't get to run the code, tests or any tool to auto-generate some reports on the code. Also, it was supposed to be a high level review and I was supposed to take 5 hours to take a look into the code and write a report on the various aspects of it. In the end, it took me 6 hours and I came up with a 14 page report. I liked this job for a few reasons:
- When I was working at www.ScrumPad.com project, we got our code reviewed by Gregg Pollack of RailsEnvy. During those days we had little experience on RoR before we started the project. So, we ended up with something that was working well, but wasn't the best one in terms of confidence and reliability. I remember the review was very well-thought and presented by Gregg and that to a great extent shaped the later works on the project.
- This was my first official assignment as a reviewer.
- I was excited to see how other people were approaching their RoR projects.
- After some googling about reviewing RoR applications, I didn't find any comprehensive suggestion and decided to go my own way.
- To get started, I took a look into the config/routes.rb file. I had the impression that, routes.rb would help me understand the resources and their relationships. But soon I discovered one potential issue as there were almost no nesting of resources and some resources had tens of actions apart from the standard RESTful methods.
- Next, I picked the resource with most number of methods and found that, it was actually taking the responsibility of 5 different resources. Although these resources were somewhat related, it made little sense to me to have such FAT controllers.
- I was also looking for logging, exception handling, ruby coding style, naming and came up with some suggestions to improve on these aspects.
- I also suggested them to modularize their controllers. They already had an admin module, but they could introduce more to clean up things.
- After controllers, I went to the models. By the time I found a few natural inheritance relations among the models. But the relations were not found in the models that ended up with a number of code duplications and similar looking branches at several places. So, I suggested them to install the relations using single table inheritance and cleanup the stuffs.
- Apart from the relations, I also found a number of self or class methods floating here and there. While ActiveRecord models themselves offer a number of such methods, its not always wise to create one without thinking. In the code I found self methods resulted in method duplication and wrong method placement at several places.
- Next coming to views, I found they did a good job with the helpers and partials. Views were better organized. But also, I found a lot, I mean a lot, of partials. Such an explosion of partials often makes it hard to find the right ones, especially for new comers.
- Going to test code, I found they had a test suite with unit, functional and integration tests. But the amount of test was a concern for me. I believe test codes are there to help good design practices and managing changes over time. I suggested them to target 80% test coverage at a minimum.
- Coming about deployment, I found they were using manual deployment techniques. I suggested them to use capistrano for deployment.
- Overall I found a lot of reinventing the wheel scenario as they could get rid of lots of code by just using commonly known and free plugins. I suggested some plugins in this regard.
- I suggested them to use named_scopes to get rid of some self methods here and there.
Also, its a good idea to know the Ruby language fluently to make use of the power of the language. A good learning resource comes with ruby installation named Programming Ruby, also available in the internet at http://ruby-doc.org
As a continuous learning tool, I also suggest keeping an eye on http://railscasts.com and http://stackoverflow.com/questions/tagged/ruby-on-rails