Blog Post for CodeMentor: How to Effectively and Politely Critique Code

Originally posted at: https://www.codementor.io/blog/code-review-best-practice-6q5slpm2e0

Programming is often a team effort for introverts. When working as a developer, you will run into decisions other developers have made that you consider mistakes.

It could be a data structure designed with three tightly bound tables. It could be a massive REST API managing all the company’s requests with obsolete technology you need to extend or replace. It could include delicate, untested code that when modified, is likely to cause both personal and technical explosion.

I have built systems that contained all of these flaws, or worked with the (sometimes catastrophic) results. Working with and learning from other programmers is simply easier when you can critique their code effectively without offending them.

Typical Code Review Process

The first step in almost any writing is to 1) write something, 2) have a group of your peers read through it, and 3) adjust to their commentary.

The importance of peer review is mirrored in the fine arts, where feedback on the semantics of your decisions is a significant part of any class. When learning to program, however, you generally receive a series of close-ended problems sized for individual consumption, and peer review of your code is rare. This did not prepare most of us for a professional life in programming.

Reading the code others have written, understanding it, and providing constructive criticism is a subtle skill. Adding to this difficulty, the individuals being critiqued are often introverts. Thus, when I am reading code written by a student or someone on my team, I have three primary concerns:

1. Team Dynamics: how do I improve my working relationship with this person and their code?
2. Code Standards: how does this code conform to how it should look?
3. Reusability/Extensibility: how easily/effectively can this code be expanded upon, maintained, and rebuilt in the future?

Team Dynamics

Team dynamics are important in software development because nobody can know everything. A software development team usually consists of developers with experience and skills enabling them to build approximately the same things.

Most software teams that I have worked on are culturally diverse. Once, my Indian boss told me, “Where I grew up, if you weren’t in the top 5% by the time you were 12, you were screwed.” Thus, as a wide variety of experiences leads to different communication styles, communication and soft skills are an important part of a developer’s skill set.

Soft Skills and Emotional Intelligence

Sometime in the 60’s or 70’s the US Army coined the term ‘soft skills.’ Now, often referred to as ‘Emotional Intelligence,’ (for further reading, the work of Daniel Goleman is fairly approachable) these skills are different from raw, memorized knowledge.

Programming consists of many ‘hard skills’ (e.g. C driver programming, MongoDB querying, and Apache server administration) — most of them are fairly ephemeral in comparison to a career. Anyone can copy JavaScript off the Internet and make a flashing button.

Convincing someone their flashing button is poorly constructed or aesthetically ugly is often more difficult. Soft skills are important in building a consensus motivating forward progression, and maintaining that consensus regarding forward progression. And the easiest, most effective way I have found to convince people to move forward is this: say something nice first.

Say Something Nice First

O’Reilly once assembled a book called, Beautiful Code, and I can’t remember many people in the book citing their own code for samples.

This means if you say something nice to somebody about their code, it may be the first nice thing they’ve ever heard about it. When you start out with, “Wow, this solves problem X very elegantly, right here,” you are building a little goodwill within your team. Also, if you have to look hard for this positive point, you’ve learned something. Often a comparatively awkward approach merely appears that way because it has an entirely different set of priorities and steps. In addition, when an approach looks ineffective to me, when I look for something worth salvaging, I end up making a specific list of issues within the code.

Be Specific in Your Commentary and Criticism

Speaking in very specific terms about awkward code is an effective way to help someone improve it, as well as learn from each other. If you look at the classic FizzBuzz post at Coding Horror, you will see that there are a thousand ways to make even the simplest program go, in all sorts of languages. Here are two pseudocode functions for finding the last day of a month:

Date returnLastDayIterative(DateTime dayIn):
    Date myDate = dayIn.date
  while (myDate.month < dayIn.month):
    	myDate.dateAdd (1 day)
    Return myDate.day.dateAdd(-1 second) 
Date ldy(DateTime i):
    Date m = i.date
    If m.dayOfMonth > 28:
    	m = m - 3
    	m.month = m.Month + 1
    	m.day = 1
    	Return m.dateAdd(-1 second)

What are the good and bad things you see about each?

Algorithm 1 is linear; it’s a single for-loop in code, doesn’t have much logic, and gets the right answer every time. It’s also cross-calendar compatible for non-Gregorian calendars.

Algorithm 2 is probably more efficient computationally, but depends on months being of uniform size within 3 days (Mars or Jupiter months might be longer) and requires two library functions (month_add and date_subtract).

Both find a solution. On balance, since date-time functions can be heavy when applied to large datasets, I personally prefer algorithm 2 for its linear performance results. Semantically, these are simple functions but if I had to maintain one, I’d probably prefer to rewrite the first function over the second. To engender this kind of focused, productive communication, what will often help is code standards.

Code Standards

Not every team has formal code standards (this is an understatement).

The default standard for many teams is: “This is what manager X of programmer Y accepted as gold code on release day.”

But when you are looking at someone’s code and asking them to modify something in a way that requires their effort, code standards help. Most of the time, code standards are about things you don’t want to care about so you can focus on what matters more. Rather than worrying about whether each file is indented with spaces or tabs, you can focus on not dereferencing that NULL pointer.

Code standards will be very different for different environments, though.

When I tested internal loan software at Wells Fargo, we had hundreds of end-to-end tests to run before putting a release on staging servers. In a tiny startup, though, your flagship product may have only a smattering of end-to-end testing (or none at all) that you run once before release. Or, more appropriately, a selective battery of unit tests run by Jeeves, TeamCity, or whatever tool before auto-migration, and a small set of well-considered hand-tests.

Code Standards Will Vary By Environment

Coding standards will change per environment, language, company, end-use, and testing requirements, so a discussion of which standard to use is important and can make communication clearer.

“Hey, I see you used a couple variables here that were declared inline. Do you think we should do that, or declare them at the top of the function? It seems it would be easier to read that way, and agrees with [Agreed Upon Convention X].”

But remember, this will change depending on the situation. When you write Java, it is a completely different animal from when you write SQL.

My team once hired a developer whose experience was extensive in Java but not Python — our primary language. We handed them a standard code-sample request. Instead of the fairly simplistic Django – Python – Postgres result we expected, we received something built with Spring. There were interfaces, and abstractions, and a factory or two.

We examined it closely, because it was…baroque to us (well written Java is almost always going to look baroque to a Python programmer). Java programmers are trained to be relentlessly abstract, so their code is reusable in a wide variety of circumstances.

When you critique a (nowadays, probably logically generated) SQL block, in direct opposition, you want it to be simple.

“Can I read this, run it on a server, and know exactlywhat it does?”

If I see a SQL schema with extra layers to it, or an SQL query nesting arrays in a table, my instinctive response is to remove the complexity. This is directly different from standard Java protocols; primarily for environmental considerations.

Watch for What’s Inappropriate in a Specific Language

A simple JSON block or SQL query can cause a tremendous number of problems. If you think of all programming in state-based terms (almost all programs can be represented as Finite State Machines) the inputs and outputs that come from any program will change the state of all programs and users interacting with it.

Thus, the environment you are working within is extremely important to understand when you look at any code chunk. If you look at someone’s code, some of the easiest/most important questions to answer quickly are, “What are the inputs and outputs, and how did they change?”

Traditional database design started with hand-generated forms and reports representing data, and modern programming technique can learn from this approach. Looking at JSON input block A and comparing it to JSON output block B should allow you to ask important questions of your fellow programmer. Likewise with a series of SQL queries pointing at the tables in question.

Check if the Code Can be Easily Maintained

When you start to talk about environments and tables, reusability and maintainability become fundamentally important. Maintainability presupposes success.

“We will need to be ready to upgrade this code eight months from now because it will be useful enough to keep us all employed,” is quite a confident statement. Telling someone that you believe their code is good enough for someone to pay for it not just today, but tomorrow and next month too is a massive compliment. I suggest saying something along those lines to them, right before you say, “But I can’t tell how or why it does what it does because there is not a single comment inside.”

When you review code, you are reviewing it not just to learn and improve your skills, but to help the creator of said code improve your/their ability to change it later.

Reusability

Would I want to be responsible for modifications to this code?

A great place to start making code more flexible later is in your code standards.

“This would be more readable if your variable names were not all variations of ‘foo1,’ ‘foo2,’ ‘bar1,’ and ‘barA4X,'” is a very good comment that code standards will help with.

Modifying or understanding large chunks of code is much easier when you follow consistent formatting and naming rules, as well as syntactic conventions.

I may not have written a bunch of Perl, but I certainly can cook up regular expressions that look like it, given time to brush the rust off. However, I have found writing three lines of code that do what one line of Perl-code-like regular expressions could do can save me a day of, “Whichever idiot let me do this eight months ago should be keelhauled,” self-recrimination.

Pointing this out to someone, or at least asking them to write five lines of documentation explaining their brilliant line noise can be enlightening for both of you. And once you’re done with the simple stuff in code standards, I strongly suggest looking at the dependencies.

Reinventing the Wheel vs. Inheriting Unreliable Libraries

I don’t particularly enjoy writing code. It’s always buggy, and doesn’t do exactly what I want. It needs to be rewritten repeatedly to handle special cases or additional inputs. Quite often, someone has run into one of my dumb problems and solved it in a way smarter than I am able to:

1. Conceptualize quickly without effort, and
2. Implement without a ton of bugs in a reasonable amount of time.

Thus, I don’t write everything in machine language. Or hand-laser wafers to get the exact transistor chains I need onto a chip. Or, sometimes, write as much Python.

Sometimes, however, I pick risky libraries. Or pick libraries that don’t do what is expected, or have insufficient testing for my purposes. When I look at code other people have written, I want to know the libraries and external chunks they’re working with to see if they are appropriate to the task at hand. It all boils down to is the implementer aware of the tradeoffs involved in using the selected references?

Generalizing Code

Expansion vs. Refactoring for Compression

After you understand what a chunk of code does and what it’s built with, the fun part comes in: where can this go?

This can actually be pretty inspiring for both of you. If the code you’re reviewing has done its job properly, you can look at it with an eye for where it can solve other problems, and be generalized. For example, should someone write a function that solves the small problem of setting a default date based upon a date-based input? You can look at the kinds of defaults a system might want: next week, next month, last quarter, etc.

Conclusion

Soft skills can make the difference between shipping a product and your source dissolving into code fiefdoms. Without a doubt, when you are working with a team, you will need to look through their code.

I have found that as I have become more effective at critiquing others’ code more politely, the software I have produced has become better. The primary ways I try to support the teams I work with in critiquing their code are through improving the team dynamics, pushing for more consistent code, and trying to help developers produce code as reusable as possible.