Level Up Your Rails Code Reviews In 11 Ways

Code reviews have become the norm and there are lots of articles that talk about how they should be done, but its easy to get lost in all the details you find on the internet and lose focus on the things that matter. This article highlights many of the critical issues you want to avoid before things go belly up in merged production code. If you already have a defined process to review code, the points here might help you augment it, or if you don't have a code review process yet, this can be a great starting point which should start giving you results immediately.

1. Sanity Check

Before you start checking the code, familiarize yourself with the objectives of why the pull request was raised in the first place. Having an overview of what functionality or refactor you expect, can help you catch entire chunks of missing code in the pull request.

2. Test code and coverage

Once you start checking the PR, the #1 thing to look for is adequate unit test coverage. Its more important than code quality because software can work fine with compromised code quality too, and can be refactored later. But not having test cases is like driving blind, you'll never know when changes to the code will break functionality without proper coverage.

Sample a few unit test cases and check them thoroughly, then check descriptions to see if enough coverage of the functionality is present.

Also look for end-to-end coverage of test cases, i.e. every component of the request-response cycle or the jobs are covered.

3. Performance

Look for things that can severely compromise performance in the codebase.

An easy way to do this is to look for loops, and assess the amount of iterations that the loop will perform. Any loop that runs through more than 1000 records will slow down the code depending on the logic thats inside of the loop. Nested loops should be scrutinized more carefully.

Also watch out for hidden loops because ruby can deceptively hide tons of loops in single lines of code, e.g. sort.uniq.flatten

Check if any objects will last the entire lifetime of the application. Examples of this include Singletons or Class variables. Check if they are required and check if they might end up storing a lot of data and eat up memory over time.

4. Concurrency Issues

Examine the code for concurrency issues. Imagine that every endpoint can be hit concurrently and think of what might happen if two simultaneous calls hit the app at the same time.

Easy ones to spot would be a lack of database transactions around an operation that involves multiple update queries.

Another one which is often missed is the addition of unique constraints in the database because activerecord models cannot ensure uniqueness with concurrent requests.

Watch out for concurrency issues in transactions. For example, incrementing a counter stored in a field in a table is prone to concurrency issues.

5. Security

A few simple checks can eliminate a lot of security issues which most people encounter with rails apps.

Ensure that unnecessary parameters are not accepted by the APIs.

Look for any parameters that are directly used in Raw SQL queries and ensure that they are sanitized before use.

Watch out for use of parameters being used dynamically, e.g. is a parameter being used as a method name via ruby's send method? If so, at the very least, introduce one level of indirection by mapping parameter values to method names which are different.

Ensure that you have a gem audit in place so that gems are upgraded as and vulnerabilities are discovered and fixed in them.

6. Code Quality

A few pointers on this outside of the usual rubocop/rubycritic checks:

Is code in the right place? What a class is responsible for should be encapsulated within the class. The simplest violation of this I see in code is the use of queries outside the model, instead of encapsulated them in ActiveRecord scopes.

Are all the names concise, precise and differentiated from other names? Are you able to clearly figure out the responsibility of a class or method when you read the name?

7. Control Flow

Is the flowyet of code simple and intuitive to understand?

Are methods and classes handling a single responsibility well rather than juggling a bunch of unrelated responsibilities?

Have methods been modified just to fit a use-case instead of code being refactored to reflect a clean design?

Is the control flow simple? No flow should be more complicated that necessary. If you see inheritance, dynamic invocations of classes and methods, or meta-programming/DSLs, you should be doubly careful and ensure that these constructs are actually required. There's always a very specific reason behind the creation of complex programming constructs, and usually there's no other way to do it. Computer scientists are pretty laconic when they design languages.

8. Error Handling and Supportability

Are errors being ignored instead of failing where necessary? Watch out for inappropriate use of &. and rescue nil.

Are there enough logs with consistent formatting to ensure that tracing errors is easy? How would the operation look on the logging dashboard?

9. Deployment

Check deploy scripts to see if everything would still function well, web server configurations, Jenkins configuration etc.

Convention over configuration - look for appropriate defaults for any configuration or environment variables.

Look for slow migrations, which can hold up deployments and see if they can be optimized.

10. A dose of pragmatism...

All of this might sound like a lot, but the idea here is to list out potential problems with code. You should use only whats important based on the PR. A simple bugfix might not require all of the checks. Use them judiciously and appropriately.

11. ...And some good old team spirit

Don't forget to instill a good team spriit when you do PR reviews. Its easy to slip into the habit of trashing code written by developers, but the better way to do reviews is to help your developers understand the importance of doing it and trusting them when possible. Don't hesitate to relax any of the criteria once a developer understands the importance of that criteria and incorporates it into their deliveries.