Best Practices for Code Reviews

This is an evolving collection of things I have learned over the years about reviewing teammates' code. Note: the important words in the last sentence is teammates. As in, we're on the same team, so we have the same overall goals–however much our individual goals differ.

Do you have corrections or suggestions? Send me an email.

Copyright © 2024-2025 Jim Hamilton. All rights reserved.

• Your code is yours while you're writing and debugging.  Once the code is merged, the entire team owns it.
• When a bug surfaces in production code, the person who wrote that code may be on vacation or at her next job.
• Reviews should be completed in a reasonable time. A one-line fix should rarely take more than a few minutes to review. (A 10,000-line merge is a different story.) If a review takes too long, it can go stale.
• Reviews should be as small as practical. 10,000 lines in a single merge is not small.
• Inevitably, one or two team members will get the reputation as the “best” reviewers, and therefore will receive the bulk of the review requests. (This is also known as “No good deed goes unpunished.”) Spread the work around. Not only does this keep the best reviewer from “burning out,” it also generates more and better reviewers.

Definition:  A merge request becomes “Stale” when subsequent (accepted) merges would result in a conflict with the stale merge request.

Zero is OK for a “trivial” change.  What’s trivial? It depends:

• Changing a variable/constant name – especially using the IDE’s refactor capability
• Removing dead code – especially when IDE points to unused code
• Removing commented-out code
• However, if the team’s rules say there are no commits without a review, even a trivial change requires a review. (Because I said so, that’s why.)

One is the “normal” level.

Two is appropriate for projects, or merges, that are deemed “critical.” That word is, of course, subjective. One example: a hot fix for a just-discovered bug, that’s going to ship today. The more eyes on it, the better.

Three is probably excessive. (We don’t want merges to go stale waiting for the third approval.) Also, the more people required to examine a merge request, the more likely someone asked to approve will be unfamiliar with the project. That’s not necessarily a Bad Thing™, but a certain level of engagement with the team will give the reviewer more interest in getting things right. 

Five is right out!  Diminishing returns and all that… 

(For remote teams, “over a Zoom call” still counts as “in person.”)

Here’s how not to do a code review:  NCIS 2 IDIOTS 1 KEYBOARD 

That said, it’s often better to do your reviews in person, so the reviewer can ask the code owner questions interactively. Just leaving comments in the gutter of a merge request can often lead to misinterpretation (and even hurt feelings (developers do have feelings, right?)). This also helps keep merges timely.

One more thing about “in person” reviews: modern IDEs are very good at static analysis, which can find many types of bugs for you. While the reviewer has the code up in Github (or whatever tool you’re using for merges), the author can have the code up in the IDE, sharing any warnings that appear. Or the reviewer can look at multiple windows, with eyes darting back and forth; these IDEs make warnings easy to see at a glance.

Some of these are contrived examples. Some are from actual, production code. Please don’t let these pass your code review.

Sometime in the mid-1990s, I stumbled across the following production C code:

unsigned long reverse(unsigned long theWord) {

  unsigned long result = 0;

  int i;

  for (i = 0; i < 32; ++i) {

         if (theWord & (unsigned long) pow(2.0, (double) i))

        result += (unsigned long) pow(2.0, (double) (31 - i));

  }

  return result; 

}

We won’t even mention the assumption that the type long is 32 bits; I’ll just assume that this was correct for the time and place. Also, I’ll be kind and assume the author understood floating point math but also knew that integral powers of 2, from 0 through 31, actually can be expressed exactly as doubles. But I will not accept that the author thought this “algorithm” was more efficient than bitwise & and | operations or shifts.

[For what it’s worth, you can find this code snippet on a website called Computer Stupidities. Plagiarism? Not really, because this was my contribution. – Jim H]

Many years ago, I was interviewing at a financial data firm in New York. In my first round, the interviewer handed me a printout of a C function, and asked me to give it a code review.  His next sentence was, “Don’t worry, I didn’t write it, you won’t hurt my feelings.” Philip, I think you were being modest, because this function was a stunningly beautiful work of art, in the medium of awful code. It included the following for loop:

for (a > 5; a < 100; a++) {...}

…which was the very first thing that caught my eye.  “Uh, that initialization clause (a < 5) does nothing.” (I got the job, by the way.)

We often put debug or trace logging in code – but sometimes we don’t think things through. Consider this Java code:

private void logArray(Object[] array) {

  int counter = 0;

  for (Object obj : array) {

     LOGGER.debug("object {}: {}", ++counter, debugCalculationOnObject(obj));

  }

}

 // debugCalculationOnObject might take a loooooong time 

private String debugCalculationOnObject(Object obj) {...}

OK, so, I wanted to see what was going on with that array, and it took a long calculation to do it. Now, my code is working, and I want to drop my LOGGER level back to INFO. But logArray(Object[]) is still going to take a long time to do absolutely nothing.

Here’s the revised method, that does the Right Thing™:

private void logArray(Object[] array) {

  if (!LOGGER.isDebugEnabled()) {

     return;

  }

  int counter = 0;

  for (Object obj : array) {

     LOGGER.debug("object {}: {}", ++counter, debugCalculationOnObject(obj));

  } 

}

 // debugCalculationOnObject might take a loooooong time  (and might not!)

private String debugCalculationOnObject(Object obj) {...}

Beware of logging inside loops!

isDebugEnabled() will exit the method if debug tracing is turned off. One could just as well call LOGGER.isDebugEnabled() just before LOGGER.debug(); and one should, if any extra work happens as a result of calling LOGGER.debug().

There are similar methods on Java loggers for isTraceEnabled(); isInfoEnabled(); etc. Check for the level you're actually logging. (If you need to know the order of levels, it's Fatal, Error, Warning, Info, Debug, Trace. Setting a log level enables all previous levels.)

Consider autoboxed numbers in Java:

Integer first = 256;

Integer second = 256;

if (first == second) System.err.println("they're equal");

else System.err.println("they're different");

Perhaps surprisingly, this code will print "they're different" to the console. That's because == is actually comparing the two object references–and they really are different. If we instead write:

Integer first = 256;

Integer second = 256;

if (first.equals(second)) System.err.println("they're equal");

else System.err.println("they're different");

we will see "they're equal," because Integer.equals() actually tests the boxed values.

There is a similar (but not identical) problem in C++:

string first = "A funny thing happened"

string second = "Because I say so!"

if (first < second) cerr << "obj-obj: as expected" << endl;

if (first < "Because I say so!") cerr << "obj-stringliteral: as expected" << endl;

if ("A funny thing happened" < "Because I say so!")

    cerr << "<we have no idea what happens here>" << endl;

What is going on here?

• The first comparison actually calls operator<(string, string). This does a lexicographical comparison between the strings.
• The second comparison calls operator<(string, const char *). This also does a comparison, and acts as expected.
• For the third comparison, forget about C++, and think about C. This statement actually compares two pointers. If the address of the 'A' in the first literal happens to be less than the address of the 'B' in the second one, you'll get the expected result. But, because these are independent chunks of memory, you have no idea which is at a lower address. (As some would say, in this case it's legal for the compiler to make demons fly out of your nose.)

All this said, we could write operator<(const char *, const char *), and have it do the right thing. Please don't do that! Your reviewer (and your successor) will not expect that comparison to work, if she knows the rules for pointer comparisons in C and C++, and won't know to look for that operator overload.

Let's revisit the "primitives vs. objects" issue from Java. In Kotlin (in the JVM), declaring a var or val to be Int will mean it's treated as a primitive. A var or val declared to be nullable (that is, Int?) will, instead, be treated as the boxed value.

val first: Int = 256

val second: Int? = 256

val third: Int? = 256

if (first == second) println("equal")

if (second == third) println("still equal")

Yes, comparing two boxed values in Kotlin will work. Why? Because Kotlin uses operator overloading. In the case of comparing two Int? objects, the == operator will call second.equals(third), which will return true.

We love our design patterns, those repeatable architectural solutions for common problems. While all code is repeatable, and therefore could be called a pattern, not all patterns are worth repeating. The bad ones are “anti-patterns.” Here are some common anti-patterns (but see also the Anti Patterns Catalog):

Remember the SOLID principles? The S in SOLID is for Single-responsibility. A God Object is one that violates this principle. Consider an object design for a vehicle. What features do vehicles have? Some are specific to particular subclasses of vehicles.

• Chassis
• Wheels 
• Axles
• Doors
• Windows
• Keel (boats)
• Lifeboats (larger ships)
• Wings (aircraft)
• Trailer Hitch
• Periscope (submarine)
• Torpedo launchers (warship)
• Armor (tanks)
• Tracks (bulldozer)

It’s true, all of those items could go into a Vehicle class, but we don’t really want to carry a lot of those around for (say) a sports car; and a submarine would most likely not have any windows. Let’s refine this design:

Steering

• controller

(sub-classes include rack & pinion, rudder, etc.)

Axle

• List<Wheel>
• steerable?

Vehicle

• Type
• Steering

Derivatives of Vehicle:

Roadable extends Vehicle

• List<Axle> (at least 1 steerable)
• List<Door>
• Engine

Car extends Roadable

• Steering
• PassengerSeats
• AudioSystem

Truck extends Roadable

• Steering
• CargoArea
• TwoWayRadio

Bulldozer extends Truck

• List<Track extends Wheel>
• Bucket
• Backhoe

Boat extends Vehicle

• Keel
• CargoCapacity
• PassengerCapacity

Lifeboat extends Boat

• Oars
• Food and Water
• 2-way Radios

Ship extends Boat

• Bridge
• Galley
• List<Lifeboat>

Submarine extends Boat

• Hatch
• Galley
• Periscope

Aircraft extends Vehicle

• List<Wing>

Jetliner extends Aircraft

• List<Turbine>
• PassengerSeating
• Lavatories
• CargoHold

You get the idea.

You need to implement a feature quickly. There’s some code in another module that’s almost exactly what you need.  You have two choices: you may either:

1. Refactor the existing code to make it reusable; or
2. Copy the existing code, paste it to a new location, and tweak it to fit the current need.

Choice 2 is the anti-pattern. How did we get here? Usually, copypasta is “justified” by claiming that a refactoring operation is error-prone – certainly true, if not done with care, if the offending developer does not understand the implicit assumptions, the original usage could be compromised. Bite the bullet anyway, and refactor. Copypasta has several implicit problems:

• It makes the code review more tedious (“Didn’t we do this already?”)
• It violates the DRY principle (well, yes, when you repeat yourself, you are repeating yourself!)
• It violates the KISS principle (Keep It Simple, Stupendously?)

If you absolutely can’t refactor right now, and it’s the very first time you’re duplicating a particular code segment, at the very least put a TODO comment in each instance of the code, pointing to the other. You are incurring technical debt, so make sure you have a plan to retire that debt.

This is similar to "cargo cult programming," wherein an inexperienced coder just copies code because it appears near other code that he actually wants to copy (borrow/steal). "Good artists copy; great artists steal." – Pablo Picasso

An operation failed, but we’re not saying where or why it failed. Maybe we’re not even saying that it failed. Examples:

In C++:

try {

   // do something } 

catch (...) { }

In Python:

try:

       something  

except:

       pass 

In Java:

try {

  // do something 

} catch (Throwable t) { }

… or maybe there was an attempt:

try {

  // do something 

} catch (Throwable t) {

   LOGGER.warn("something went wrong");

  // or:

   LOGGER.warn(t.getMessage()); 

}

Java’s exception handling is exceptionally robust. (See what I did there?) Probably more than 9 out of 10 times, the stack trace and the exception type are enough to tell you what went wrong, and either fix it or handle the case where it occurs. In that last snippet, we had all that information, and just threw it away. Here’s a better way:

try {

  // do something 

} catch(Exception e) {

   LOGGER.warn("Exception caught: ", e);

 // logs the type and stack trace 

}

Even better, when multiple exception types can be thrown, they can be handled differently:

try {

  // do something 

} catch (NumberFormatException nfe) {

  // handle that 

} catch (FileNotFoundException fnfe) {

  // handle that 

} catch (Exception e) {

   LOGGER.warn("Something I wasn't prepared for happened: ", e); 

}

In none of these cases did we catch Throwable; those are generally more serious errors, requiring some reworked code. However, some code somewhere will catch Throwable, even if it’s the top level of the stack, to abort the program. Which brings up another principle: exceptions should only be caught by code that knows how to handle them. Uncaught exceptions will happily unwind the call stack until they are caught. “There’s no need for me to continue if I can’t handle the problem; I’ll make it Somebody Else’s Problem.”