Code review as a tool to improve quality. Part 2 – What feedback to provide

Other posts in this series

Part 1 – how to provide feedback
Part 3 – why should you care?

In the previous post we discussed what a code review is, and some guidelines on how to deliver feedback. Also, importantly – what goals we should try to achieve when reviewing code .
We defined the first, most obvious (and difficult) goal as:

1. Quality control
The reviewer tries to ensure that the proposed changes meet certain bars of quality. If it doesn’t, they may suggest ways to improve the code’s quality.
(“Code quality” can be subjective, and I’ll try to define what (I think) it means in the next posts.)

Therefore, in this post, we’ll explore how to spot “low quality” code that can be improved on.

What code quality is not (AKA you’re not an inefficient, overpriced linter)

Don’t waste time checking for things that can be automated (style and formatting violations). Instead:
1. Plug a linter into your test suite / CI pipeline
2. Have a team-wide discussion about tabs vs. spaces, curly braces on their own line or not, and all that fun stuff
3. never think of it again.

Don’t distract yourself from the purpose of the current code at hand with linting.

What I would suggest you do focus on, are the fundamentals of maintainable code and design. Those include – SOLID principles, consistent architecture and design, and code smells.
I’ll briefly discuss those in this post. I’ll provide links for further reading on each topic, as each one of them is worthy of a blog post (or book) of its own. I’ll merely provide a taster for each one.

Well-known code smells

These are relatively easy to identify, so are a great starting point.
If you take just one thing from this post, take this: Familiarize yourself with some common code smells (use the excellent refactoring guru), and look out for those.
(Note: these are highly dependent on context and situation. Some smells result from fixing other smells. You’ll have to use your judgement as to what applies to the code at hand.)

Example:

public updateUserDetails(UID userId, string newFirstName, string newLastName, string newStreetAddress, string newCity, string newState) 

This method suffers from the “long parameters list” smell.
A reviewer can point out that smell, and help guide the author to its solution (possibly using a User object rather than a list of fields, or a parameters object).

Well-known code anti-patterns

These are both relatively easy to identify, and removing them provides high returns.
Familiarize yourself with some common anti-patterns, and look out for those. (Some examples – pokemon exception handling, using inheritance where composition is preferable)
The fact that they are generally agreed to be “bad” will also help you convince the code author to change them.

Example:

public isValidDate(string str) {
  try {
     var dummy = parseDate(str);
     return true;
  }
  catch (FormatException e) {
    return false;
  }
}  

The above method employs the known anti-pattern of using exceptions for flow control.
A review comment here might point out the anti-pattern, and ask the author how it could be changed.
(remember from our previous discussion – prompt the author, do not spoon-feed them)
A solution to the above example could be:
Move the logic that causes a FormatException to be thrown from parseDate to isValidDate. change parseDate to call isValidDate.

Well-known architectural anti-patterns

Similar to the above, these are cases where the proposed code changes go against the system’s architecture.
For example – tightly coupling modules that should be independent of each other. breaking well-known architectural principles (such as placing UI-related code in the domain model).
Example:

private submitButtonClicked() {
  var user = UserRepository.get(usernameField.text);
  updateResult = UserController.updateUserDetails(usernameField.text, firstNameField.text, lastNameField.text);
}

The above method breaks the system’s layer model. It invokes a repository method directly from the view.
A reviewer may point that violation out.
If necessary, she can suggest moving the call to the repository to a more appropriate layer (controller layer, for example).

Loose coupling

A component has, or makes use of, little or no knowledge of the definitions of other components.
Components that do not adhere to this principle make the system harder to change. That’s because changes to component A will need to ripple out to components B and C that are coupled to it.
Example:

var streetAddress = user.Address.Lines.split('/n')[0];

The above code is tightly-coupled to the implementation of the User’s address field. Any future changes to the implementation of Address will need to take this code into account.
A reviewer may point this out (with the goal of moving this logic into the Address object itself).

Single responsibility

A component should have responsibility over a single part of functionality
(should have one, and only one, reason to change).
Useful question to ask yourself as you review a component : “What’s the role / responsibility of this?”. Look for the word “and” in the answer.
Example:

class UsersManager {
  public search(string name);
  public update (string userId, string newName, string newEmail);
  public resetPassword(string emailAddress);
}

The above class is an example of a “god-object” ; it has many reasons to change.
update will change if the fields of the User object change.
search will change as we add more searchable fields.
resetPassword will change with any changes to our authentication mechanism.
A reviewer may suggest breaking up the different responsibilities into different objects. A common pattern to do that is employing domain service objects.

Leaky abstractions

An abstraction exposes implementation details that are not relevant to its interface.
Or: a user needs to be aware of the implementation details when using the abstraction.

Useful question to ask as you read the code: “As the user of this class / method, what do I have to know?” In the answer, look for anything that is related to “how”, rather than “what”, the component does.
Remember – to use a component, you shouldn’t have to know how it does its job.
Example:

class Payment {
  public charge(double amount, string stripeMerchantID);
}

Users of the above class have to know that charging a payment is done by stripe.
A reviewer may request that references to ‘stripe’ be removed from the class’s public API. A common way to do this is to –
1. Separate the creation and usage of instances of the Payment class (for example – using inversion of control)
2. configure the class at creation time with the necessary stripe credentials.

In that way, the actual usage of this class be free of references to the underlying implementation.

Sane and consistent APIs

Good and consistent API is such that:

• The inputs and outputs are consistently the same structure / types.
• No unforeseen side-effects (such as modifying an input parameter)
• the API client doesn’t need to do special parsing to pass values in and out of the API

Examples:

newUserID = createUser(params)[0]
the createUser method’s result requires the client to do some weird parsing.
public makeCharge(Money amount, double discountAmount)
this method represent an amount inconsistently – once as a Money object, and once a plain double.

Naming

This isn’t necessarily a category in itself.
However, paying attention to how things are named can help us spot a smell of one of the other categories.
For example –
public makeChargeAndMarkAsPurchased() indicates that the method is not singly-responsible.
The name mongoCacheProvider.store(value) leaks the fact that our cache is implemented using mongodb.
The name SendRequestToStripe(amount, merchantID) indicates how the method does its job. A better name would describe what the method achieves. For example – chargeCustomer.

Test-code smells

Test code is code in your system that needs to be maintained and evolved over time. It should not be overlooked when reviewing changes.
All the above criteria also apply to test code, especially –

• Single responsibility – a single test should verify one thing.
  a good metric is to make sure that each test contains exactly one assertion.
  Another rule of thumb is to look at how much faking / stubbing is happening in the test. If a lot of dependencies have to be faked, this may be an indication that the subject under test is doing too much.

• Leaky abstractions – the test checks against the public API of the subject under test. i.e inputs and outputs, or side-effects, such as writing to DB.
  look for tests that test the SUT’s implementation (i.e the SUT calls a certain method on another object)

• Naming – the test description talks about the object’s public API, rather than the implementation. i.e assertReturnsANewUserRecord rather than assertCallsUserRepository

Commit hygiene

separate commits can (and should) be used to make proposed changes clearer.

Atomicity (or single responsibility)

A commit should be a small, self-contained change. Applying that commit should achieve a single, well defined, goal.
A single “implemented the whole world” commit defeats the very purpose of having commits at all.
Ideally, a commit contains a small set of functionality, along with its verifying tests.

Commit message explains “what” and “why”, rather than “how”

A commit message should convey what functionality / business goal this commit implements. When necessary, also – why the proposed changes were made in that way.

A useful trick is to write the commit message as the completion of the sentence “When applied, this commit will..”.
For example – when applied, this commit will “add support for logging in with google”.
You may also add clarifications regarding the chosen implementation. i.e “made the login functionality async, since google authentication works asynchronously”. This will clarify why the change was made in the way that it was.

Some examples that a reviewer would want to comment on:
WIP , it works, trying something – these don’t explain anything.
Deleted unnecessary code from User class – doesn’t add any information. A reader can understand that code was deleted by just looking at the code changes.
A better commit message would explain why this code was deleted. For example: Removed support for MFA explains the business functionality that’s achieved with that change.

Some VCSs support re-writing the commit history.
you can (and should) encourage your team members to do that to create a coherent story that’s easy to follow.

Summary

These are the main things I look out for when reviewing code. It may seem like a lot, but, with time and practice, they become easier to spot.
However, a rigorous review still takes up more time than you may be used to, even for an experienced practitioner.
I’ll try and convince you that this time is well spent, in the next (and final) post of the series.

Read next

Part 3 – why should you care?
Part 1 – how to provide feedback