Ruby on Rails Code Quality Checklist

Emeryville, CA: September 16, 2008 In my experience, Ruby and Ruby on Rails has been one of the most difficult language/framework combinations to truly master.

September 15, 2008 · Topics: software engineering, product management, startups

Emeryville, CA: September 16, 2008

In my experience, Ruby and Ruby on Rails has been one of the most difficult language/framework combinations to truly master . For someone who grew up on C, C++ & Java in the majority of their training, Ruby has hugely different (and better!) ways of OO design, and the Rails framework has a lot of opinions to be understood and remembered. As long as it’s taken to master them to the level I have - and I’m sure there’s still a long way to go - I love it and won’t be going back.

I have a sneaking suspicion that as Ruby on Rails keeps rising in popularity, there will be lots of developers stuck in the Java-style OO mentality, lots of developers who are just learning; and that’s a Very Good Thing. It’s also a bad thing, because poor code begets other poor code, when published and viewed by others.

As ThriveSmart hires more developers - not all of them Ruby or Ruby on Rails experts - there’s a growing need to ensure that code and design strategies maintain an extremely high level of quality across different projects. My good friend Dan and I assembled this checklist that all of our teams are expected to sign off on for each of their projects. It’s an evolving list, but here’s a snapshot of it.

Ruby on Rails Code Quality Checklist

Each controller action only calls one model method other than an initial find or new.
(Make custom .new or .update methods in the model with all necessary). …
Only one or two instance variables are shared between each controller and view. …
All model and variable names are both immediately obvious (to a new developer) and as short as possible without using abbreviations. …
All custom “finds” accessed from more than one place in the code use named_scope instead of a custom method. …
A .find or .find_by_ is never called in a view or view helper. …
There is zero custom code that duplicates functionality of a built-in function in rails. …
Code has been aggressively DRYed during development. …
All functionality used in two or more models has been turned into a library/module. …
All logic duplicated between two or more apps has been turned into a gemified plugin. …
STI is not used anywhere …
Every design choice should yield the most simplistic design possible for the need of users at the current time.
No guesses for future functionality were designed into the application. …
Close to full test coverage exists at the highest level of the application: on and between controller actions.
Coverage is highest for code used by the most number of end users. …
All tests pass before code is merged into a shared repository. …
Every fixed defect on a deployed product has tests added to prevent regression. …
Every plugin installed has been code reviewed. …

I certify that all of the above is true for my project.
[Printed Name] Signature

Explanations

The hope is that every item on the checklist is so obvious to the more experienced Ruby on Rails developer, that it’s not worth mentioning - but that to new Ruby on Rails developers, the items on the list are non-obvious, and require some explanation. So here goes:

Each controller action only calls one model method other than an initial find or new. Make custom .new or .update methods in the model with all necessary.

Fat models and skinny controllers is the expected methodology of coding in Rails - but that term is too open to interpretation for my taste. In almost all circumstances, you can actually push all of the logic into your models, so your controllers look identical to the controllers generated by script/generate - you just change the generic .new calls and .update_attributes calls to similar but custom methods in your models. A simple example is when there’s additional logic when the attributes are updated by a particular user: @foo.update_attributes_by_user(params[:foo], current_user).

The only time extra logic should be in the controller and not in the model is if it leads to rendering a different action or redirects differently.

Only one or two instance variables are shared between each controller and view.

Instance variable hell - when a lot of instance variables are shared between a controller and a view - is easy to do in Rails. It’s also potentially dangerous to performance, because you can end up making duplicate calls on associations unknowingly. Instead, your controller should only manage one instance variable - and perhaps a second for the current_user. That way, all calls to associations are loaded “on demand”, and can be instance-variable-cached in a single place.

This methodology also works out well for fragment caching, because you can check caches in views before actually loading associations.

For example, instead of having your Blog controller create an instance variable for both @post and @related_posts, just make a single method, @post, and give your Post model a related_posts method, so you can just call @post.related_posts in your views.

All model and variable names are both immediately obvious (to a new developer) and as short as possible without using abbreviations.

Naming is hard. Particularly for developers who are immersed in an application - what’s obvious to you when you’ve loaded the entire context of the application into your brain, will not be obvious to you later or to a new set of eyes.

One of the greatest things about Ruby and Ruby on Rails is that names are short and obvious, and readable to a fresh set of eyes. Don’t ruin this immense strength!

If you can’t think of an ingenious, clear, and short name immediately, finish coding your method, and then try to unload all the context you have while deep in coding. Put a TODO in and rename your variable or method name at the end of the day.

All custom “finds” accessed from more than one place in the code use named_scope instead of a custom method.

If you don’t fully understand the power of named_scope , you aren’t allowed to do development for us. Named_scope should change how you write any and all of your .find code.

If it’s not obvious why this rule is here, you need to go back and fully understand the power of named_scope . Really. Chained named_scopes are the most readable way of doing any finds with conditions, so you must use them.

A .find or .find_by_ is never called in a view or view helper.

If you’re using a .find or .find_by_xyz on the base of a model in a view, there’s a 99% chance there’s a better way to do it - and you should do it.

At the very least, you should be using a named_scope, and calling that from the base of the model. More likely, you should be calling an association or custom method from your model (on which you can chain a named_scope).

For example, if you want to find all recent articles for listing in a view of a blog post (@post) you might have done: Articles.find(:all, :conditions => …). You should be doing: Articles.recent (named_scope) - or even better, @post.related.recent (chained named_scope).

There is zero custom code that duplicates functionality of a built-in function in rails.

Although this is a general programming rule to live by, I see it the most with Rails. Unlike other frameworks and languages, Ruby on Rails provides many, many helper functions for displaying things readably, and doing common tasks. But they are typically tucked away (as they should be) into a bunch of different files - modules that are mixed in at the appropriate time.

Thankfully, the Rails code base is VERY well coded - even where it lacks documentation. To help find methods that you can use, I suggest keeping a copy of the rails source in your text editor or IDE - and at any time, you can do a full-text search on the entire rails source. Of course, just download the archive version from github .

Code has been aggressively DRYed during development.

The way Ruby has been designed, it’s overly easy to DRY your code continuously as you do development. Make helpers. Make modules. Make plugins.

Just keep doing it.

All functionality used in two or more models has been turned into a library/module.

Perhaps the biggest advantage of Ruby over Java-style languages are mixins (modules). Don’t ignore that - use it to it’s full advantage.

In Java-style OO, to get shared functionality between classes, you usually have to sub-class. This is not the case. In Ruby, you can mixin functionality from many different modules into any class or object - even at runtime. Understand modules well , and use them to share duplicate code between models (and controllers)!

**All logic duplicated between two or more apps has been turned into a gemified plugin. **

Now that plugins can be gemified , there’s no reason not to make plugins. They’re easy to make, and now very easy to re-use and manage across deployments.

Unlike other frameworks, Ruby on Rails plugins are extremely light-weight and easy to code. It’s worth making a plugin, even if it’s just a 5 or 6 line module.

And most likely it makes your code easy to migrate if you move to other Ruby frameworks, like Merb.

STI is not used anywhere. At all.

I’ve seen my fair share of rails apps. I have never seen a Rails app where using STI (Single Table Inheritance) was the correct design decision. Ever. I’ve seen them most often when someone is new, and thinks STI is a great way to share code.

But in rails, it’s so easy to add columns to tables, and share code between models without STI.

Instead you should be generating different models, and using a module or two to any correct logic between them. Use common partials to share view code. If you use STI, you will forever bind the two+ models together in ways that can be very hard to undo - a data migration is never fun.

Polymorphic associations, however, are encouraged!

Every design choice should yield the most simplistic design possible for the need of users at the current time.
No guesses for future functionality were designed into the application.

Another way to put this is The Rails Way. And this paradigm is just as much for Project Managers as it is for Developers - but both types of people on a project have a responsibility to ensure that this is followed.

Although Chapter 4 of Getting Real explains it better than I ever could - to sum it up. If you make guesses, rather than designing to facts, invariably, some of those guesses will be wrong. Some might be right, but any designing and development that are for wrong guesses make things more complex, harder to fix, and harder to improve.

Instead, code to what you know is true, launch early, watch users, and iterate quickly. That’s on of the major benefits of hosted software (web apps) over distributed software (desktop apps) - so use it to its full advantage.

Close to full test coverage exists at the highest level of the application: on and between controller actions.
Coverage is highest for code used by the most number of end users.

Although unit tests can be important for iterating features, for most young Rails apps, models are more simplistic. What you should make sure of is that all points of interaction for users are well tested, and stay un-broken from release to release.

The easiest way to prioritize coverage of tests? By the portion of your end users that will use a piece of code.

All tests pass before code is merged into a shared repository.

This should have been true at your last job or on your last project. If it wasn’t, it is now.

We encourage you to keep your own local repository/branches for backup purposes - that’s what git is great for. But as soon as we have a launched product, never merge your code from your branch, into a branch that other developers use before all tests are passing.

Every fixed defect on a deployed product has tests added to prevent regression.

The worst thing you can do in this business is to have the same problem twice. Prevent it - our clients and users will usually understand if a bug happens the first time. Not ever, if it happens again.

Every plugin installed has been code reviewed.

Most plugins in the Ruby on Rails community are at least good, if not great. However, the number of poorly designed plugins will increase. Always review the actual code of the plugin. In general, the simpler/smaller its code base is, the better. The more well-tested it is, the better. The easier its code is to understand, the better.

Do you understand how it works?
Do you understand its rough performance?

That’s it! Please keep in mind that this is an evolving list. It will change, especially as Ruby and Rails changes.