The following is adapted from an internal writeup I did for my team on the topic of code review. It has been shaped by my experience as a developer and engineering manager in the particular technical, social, and business contexts of my career. A year ago, this document would have looked different. A year from now, it will hopefully look different too. While I think some of these points are applicable in all contexts, I make no claim that this is how everyone should do code review.
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.
Things To Look For In Code Review
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.
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?
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: truein 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 team 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
def some_collection_filtering_method @collection_from_class_args.select(&:meets_some_condition?) end
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
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
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?
Not capturing the output of
update. 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, via
update!? Be careful of this in tests: if we are doing an
updatein a test but not saving the return value and ensuring it’s
true, how are we sure our test setup is correct?
find_eachwhen iterating over collections of unknown size.
Not scoping queries. There are cases where it’s OK to do this, but they are rare.
You can discuss this post at Hacker News.