How to evaluate the quality of Rails code?

Question

In a code review, what do you look for to assess a developer's expertise? Given an opportunity to look at a developer's work on a real-world project, what tell-tale signs are a tip-off to carelessness or lack of experience? Conversely, where do you look in the code to find evidence of a developer's skill or knowledge of best practices?

For example, if I'm looking at a typical Rails app, I would be happy to see the developer is using RSpec (showing a commitment to using test-driven development and knowledge that RSpec is currently more popular than the default TestUnit). But in examining the specs for a Rails model, I see that the developer is testing associations, which might indicate a lack of real understanding of Rails testing requirements (since such tests are redundant given that they only test what's already implemented and tested in ActiveRecord).

More generally, I might look to see if developers are writing their own implementations versus using widely available gems or if they are cleaning up code versus leaving lots of commented-out "leftovers."

What helps you determine the skill of a Rails developer? What's your code quality checklist?

Answer 1

It is generally easy to determine the quality of code, and it has nothing to do with Rails or Ruby.

1. All functions should be between less than 8 lines.
2. No function should have more than two levels of indent.
3. Names should be communicative and explicit.
   3a. Variables in short scopes should have short names. 3b. Variables in long scopes should have long names. 3c. Functions in long scopes should have short names. 3d. Functions in short scopes should have long names.
4. The code should read like well written prose and have very few comments.
5. As you read the code you should not have to look up the definitions of the variables and functions being called. They should be obvious.
6. Function calls should have 3 or fewer arguments, with a preference for fewer.
7. Unit test coverage should be close to 100%, and the programmer should know the coverage number.
8. Unit tests should be short, easy to read, and easy to understand. You should be able to understand the program by reading the tests.
9. The tests should execute very quickly. Long running tests are a symptom of carelessness.

Look for signs of carelessness like commented out code, meaningless comments, needlessly shortened names, long functions, long argument lists, etc.

Ask yourself whether or not the code looks like the programmer cared.

If you want some Rails specific stuff -- look to see if there are business rules in the active objects. This is a symptom of careless design.

Answer 2

Agreed that it's important to not re-invent the wheel, but I wouldn't be too judgmental about which modules they choose. Your fussiness over RSpec/Test::Unit will likely cause you to miss some good code. Likewise with a hpricot/nokogiri, etc. You may perceive one pulling away from the other, but that may not be the perception of a talented developer (or maybe they've been implementing cool features versus writing the stale module out).

As for a quality checklist, I err on the side of beauty:

• Does the code look beautify on the page? (I realize this is subjective, but to someone who "gets it" I maintain it's the best measure of excellence.)
• Are semantics consistent throughout the project?
• ActiveModel if appropriate
• Are migrations used to their full potential?
• Scopes and relations in the proper context?
• Uses virtual attributes?
• Complex object creation is handled in a factory?
• Effectively uses meta programming to make the project clean and tidy?
• Views are logic-free?
• Finally, another subjective but important item - code feels like the developer loves the project and wants to provide it with the best care possible.

Answer 3

All you need to evaluate is if the code is easy to understand, well structured, and has logic sound enough for someone that got good grades in high-school science courses.

Everything else you mention is about knowledge or experience, and is thus something that someone wanting to write good programs will learn sooner than later.

The lack of experience is something about which agreements about compensation can be made while hiring. Carelessness, genius complexes, lack of logical thought, etc are out of scope.

By negation: If the code uses all the correct idioms and libraries, but you can't grok it easily, then at least ask the guy why his code is so hard to understand before hiring.

Answer 4

For any open sourced code, especially if it's available on Github, you can clone the repo and run metric_fu over it. Compare scores with a piece of code you wrote that you think is good.

Secondly, look for the usual practices:

• Skinny controller, fat model
  • Even better is if the developer isn't strictly adhering to one controller to one model architecture
  • Better still is if the developer breaks out business logic pieces out of the model
• Good test coverage of the model and controller
• Good approach to handling Ajax calls
• Good approach to problems that aren't CRUD

Answer 5

Running metric_fu will help point out possible red flags.

Metric_fu is a set of metric tools that make it easy to generate metrics reports. See the list of metrics the gem includes It's designed to integrate easily with CruiseControl.rb by placing files in the Custom Build Artifacts folder...