What should we look for when doing code review? How should we receive code reviews from others?
thoughbot's Code Review guide is a great starting point. It articulates principles for the relational side of code review. This is especially relevant for us in a remote environment. How we give and receive feedback is as important as the feedback itself.
The following is a very incomplete list of questions to ask yourself during code review. The items here are meant to be starting points for a conversation with the code's author. Always assume the best intentions.
- Does the PR title follow our naming convention?
- Is the commit message well-formed? See How to Write a Git Commit Message and 5 Useful Tips For A Better Commit Message.
- Given the commit message, do I, as a reviewer, understand the purpose of this code? Do I know how to manually QA it? Do I know how I'd even determine if the code does it what it claims to do?
- Has anyone outside of the author's team QA'd the feature or bugfix to verify it?
- Have all it's tests passed?
- Will it merge cleanly?
- Are there any Hound errors? Has ESLint been run on any new JS code?
- Are there any lines of code that make me scroll horizontally? If so, the line is probably too long.
- Are there tests? If not, why?
- Do the tests follow our RSpec conventions? If not, the author should present a case for introducing a new pattern.
- Are these the right tests? This can be hard to determine.
- Do the tests prove that the code does what it claims to do? Generally, there should at least be tests at the highest level possible level.
- Are the tests efficient? Do we need to hit the database that many times to prove our code works? Do we need to reload that object? Are we doing
js: true
in feature specs that don't need JS? There are so many aspects to this question. - Is the code readable (well-named variables and methods, etc.)?
- Is there duplication of business logic? This is not always a bad thing (see The Wrong Abstraction). But we should be on the lookout for duplicated business logic nonetheless.
- Does the code follow the general pattern of other code that does similar things? If not, the author should present a case for introducing a new pattern.
- Can we achieve any performance boost (however small) from memoizing?
- If writing a new background job, has the Operations taken a look at the code and approved it? What are the things they look at when determining if a background job is efficient or not? Learn those things!
- If writing an Elasticsearch query, is pagination going to behave the way the author thinks it will?
During code review we want to look for sub-optimal Ruby Standard API usage. Most of these instances will be related to misuse of Enumberable methods. Reading Refactor to use Ruby standard library will give you a good grounding in ways to make Ruby code more idiomatic.
The most common instances take this shape:
def some_collection_filtering_method
my_array = []
@collection_from_class_args.each do |item|
if item.meets_some_condition?
my_array << item
end
end
my_array
end
A more idiomatic approach would be to use #select
:
def some_collection_filtering_method
@collection_from_class_args.select(&:meets_some_condition?)
end
Similarly:
def some_collection_transforming_method
my_array = []
@collection_from_class_args.each do |item|
modified_item = widgetize(item)
my_array << modified_item
end
my_array
end
A more idiomatic approach would be to use #map
:
def some_collection_transforming_method
@collection_from_class_args.map do |item|
widgetize(item)
end
end
And if the result of widgetize
could be nil
, we could use #compact
:
def some_collection_transforming_method
@collection_from_class_args.map do |item|
widgetize(item)
end.compact
end
One should look for these instances when doing code review.
Some Rails-specific things to look out for in code review:
- Are there any unnecessary database calls? Are we missing an eager-load?
- Using
where(...).first
instead offind_by(...)
. - Using
update_attributes
instead ofupdate
. - Not capturing the output of
save
orupdate
. Why doesn't the author care if their database call was successful or not? If they don't care, should the code raise an exception if it's not successful, viasave!
orupdate!
? Be careful of this in tests: if we are doing anupdate
in a test but not saving the return value and ensuring it'strue
, how are we sure our test setup is correct? - Not using
find_each
when iterating over collections of unknown size. - Not scoping queries. There are cases where it's OK to do this, but they are rare.