Code Review Quick Wins

I came across Google’s official “Standards of Code Review” documentation and was inspired to write down some thoughts regarding a few simple things I always look for when doing code reviews. The techniques covered in this post should all result in quick, easy wins for your code base if applied when conducting your own code reviews.

Code reviews should not hinder progress. Like art, code is never really finished, only abandoned. Code is never perfect, but it can always be better. Code reviews should not seek perfection, but instead attempt to continuously improve the overall code health of the system being worked on, even if the pull request under review is not perfect.

This post isn’t meant to be a formal proposal putting forth standards for code reviews. Instead, I’d like to go over the “quick-win” techniques that I try to drive home when participating in code reviews. Let’s put on our inspector hats and dive into a few topics that really resonate with me!

Note: This post is a deviation from my last couple of posts with a fantasy football stats theme. Don’t worry though, all of the code examples will be fantasy football inspired :D

Code Complexity

We spend more time reading code than writing it. When we write code that is overly complex we increase the time it takes to read the code which increases the time it takes to maintain the code as well. When reviewing pull requests I aggressively look for overly complex code. Are individual lines too long? Are logic-clauses too nested? Are functions doing too much? Are classes being improperly used, and not really objects? Code should be quickly understood by its readers. Let’s take a look at an example:

Bad:

def get_stats_by_position(position): 
    if position == "QB":
        return qb_stats
    elif position == "RB":
        return rb_stats 
    elif position == "WR":
        return wr_stats
    elif position == "TE":
        return te_stats
    elif "FLEX" in position:
        if position == "SUPERFLEX":
            return super_flex_stats
        
        return flex_stats

The above function is rather simple, but overly nested! Now, let’s take a look at a function that achieves the same result, but is much less complex:

Good:

def get_stats_by_position(position): 
    stats_by_position = {
        "QB": qb_stats,
        "RB": rb_stats,
        "WR": wr_stats, 
        "TE": te_stats, 
        "SUPERFLEX": super_flex_stats,
        "FLEX": flex_stats,
    }
    
    return stats_by_position.get(position)

This method removes the nested if-statement in favor for a much more readable dictionary. If we put these two methods through a code complexity calculator we can see the top method has a complexity score of 7 while the second method has a much lower score of just 1!

Naming

One of the most important things in a clean codebase is naming. Clear, meaningful names of modules, classes, functions, and variables can be the difference between code that’s a joy to read vs. cumbersome text. As Clean Code states, writing well-crafted and self-explanatory software is almost as important as writing working software. Here are a few examples of good naming vs. bad:

Use Meaningful and Pronounceable Variable Names

Variable names shouldn’t leave our readers confused and guessing at what they represent!

Bad:

 pn = "Jonathan Taylor"

Good:

 player_name = "Jonathan Taylor"

Use Searchable Names

As I mentioned above, we read code much more than we write it. It’s important that the code we do write is not only readable but also searchable. When we don’t choose names that are both readable and searchable we hurt the maintainers of our code. Thus, make your names searchable!

Bad:

# What is this code doing? What is the value '1'? 
# When we finally realize what '1' means how do we even 
# search where else this value is used in our code?
# We certainly can't just search for the value '1'.
players = get_players_by_position(1) 

Good:

POSITION_QB = 1

# We can now clearly see this line gets all playes that play the 
# quarterback or "QB" position. Additionally, we can search the code 
# for other places where the constant 'POSITION_QB' is used.
players = get_players_by_position(POSITION_QB)

Function/Method Names Should Say What They Do

APIs and libraries should feel intuitive to use. In a perfect world a developer should simply need their IDE, some “code completion” suggestions and have a solid idea of what methods they need to call.

Bad:

class FantasyLeague:
    def process(members):
        # Process? What does 'process' even mean
        # to the next developer who invokes this method?


fantasy_league = FantasyLeague() 
# What does process even do?
fantasy_league.process(members) 

Good:

class FantasyLeague:
    def open_new_fantasy_league(members):
        # Open a new fantasy league containing the passed in members. 
        # It is now clear what this method does.
    
    fantasy_league = FantasyLeague() 
    fantasy_league.open_new_fantasy_league(members)

Comments

Our goal should be to write code so clean and expressive that code comments are unnecessary. Code comments should never explain the WHAT, but only the WHY in extreme circumstances.

Clear Names Should Explain the What, not Comments

Compounding on naming from above, clear, concise names also help alleviate the temptation to use comments to constantly tell the “what” in our code.

Bad:

# Calculates weekly scores, determining winners and losers for the current week
def process(player_stats):
    # Do Stuff

Good:

 def calculate_weekly_scores(player_stats): 
    # Do Stuff

Abstraction Should Explain the What, not Comments

Functions should do one thing and they should do it well. If you find yourself writing long, overly complex functions and using comments to separate pieces of functionality then a refactor is likely needed.

Bad:

def update_starting_roster(roster, new_player, old_player):
    # Check roster is valid and doesn't need players dropped
    ...
    
    # Check new player is eligible for this position
    ...

    # Swap the old player with the new player
    ...

There is a lot of logic embedded into this single function that merely swaps one player in a fantasy starting lineup for another. While all these steps are necessary to successfully change one’s lineup, the logic should be abstracted away into smaller, reusable, well-named functions. All of these comments indicate a code smell and should not make it through a code review.

Good

def update_starting_roster(roster, new_player, old_player):
    if not valid_roster(roster):
        # Notify user of invalid roster

    if not player_can_play_position(roster, new_player, old_player):
        # Notify user player cannot play position 
    
    update_roster(roster, new_player, old_player)


def valid_roster(roster): 
    # Check roster validity


def player_can_play_position(roster, new_player, old_player): 
    # Check player can play the position they are being moved into


def update_roster(roster, new_player, old_player)):
    # Update roster, swapping old player for new

Summary

In conclusion, when doing code reviews try to ensure:

• The code is now healthier than it was before the pull request came under review.
• Code complexity is aggressively eradicated in favor of simplicity.
• Names of variables, functions, and classes clearly show their intent.
• Code comments are sparingly used to explain WHY some code exists and not WHAT already clear, concise code is doing.
• Everyone is kind to each other, and remembers to compliment great code!

Happy code reviewing!