If you read my previous series on How I Git it, you know I like my git commits all nice and clean. I do this for you, my dear code reviewer. I remove the noise so that you can focus on the change. What is the purpose with code review, if you cannot see through the noise?

Asking to remove that noise is the kind of nitpicking that I want to write about.

The Noise⌗

The noise is like a cloud that obfuscates the real changes made to a code base. It consists of:

  • Whitespace.
  • Formatting changes.
  • Dead code.
  • Commented out code.
  • Moved code.
  • Refactored code.
  • Unrelated code changes.
  • Changes that are then changed again in a later commit.

They are all difficult to see through by themselves, but when they are mixed and they are many, the cloud becomes a wall.

The Origin⌗

Where does the noise come from? Well, from me (and you) of course. It is our daily fiddling with the code. The accumulation of various changes that we do over time.

I think I can safely say that no one writes perfect code on the first try. Like, never going back to add another argument to the function that they recently created… and later ending up not needing it after all.

At least I do not and I know for a fact I am not alone. While developing I will create noise. It is how I choose to deal with it that makes a difference.

The Nitpicking⌗

I talked about how I nitpick on myself in my rather lengthy series about How I Git it. Basically, removing the noise is all about creating logically atomic commits.

Actually, nitpicking is really not what I do in my opinion. But were I to jump straight into a team and start reviewing code how I personally like it to be, I can see how others may feel it is. I do not do or recommend that of course!

Instead I find it very important for a team to discuss these things, and to agree on where to set the standard. What is nitpicking and what is not?

My ultimate goal is to be able to give and receive good feedback. With a wall of noise that is just not possible. It takes too much time and brain power.

I want all important changes to really stand out. Be highlighted with red or green. So that we, as a team, may have valuable discussions about real issues and business requirements.

I believe, it is the reviewee who is best equipped and responsible for breaking down the wall of noise. Not to mention, there are often multiple reviewers in front of the wall who will all benefit from it’s removal.

The Example⌗

Say a developer is tasked with adding pre and post data processing events to the code below:

import { EventEmitter } from "events";
import DataItem from "DataItem";
import DataApiClient from "DataApiClient";
import DataProcessor from "DataProcessor";

const eventEmitter = new EventEmitter();
const dataProcessor = new DataProcessor();

eventEmitter.on("recentDataUpdated", (data: DataItem) => {
    processData(data);
});

const client = new DataApiClient();
let mostRecentData: DataItem = null;

setInterval(() => {
    console.log("Polling for new data");
    let recentData = client.fetchRecentData();
    if (
        mostRecentData !== null ||
        mostRecentData.timestamp < recentData.timestamp
    ) {
        mostRecentData = recentData;
        eventEmitter.emit("recentDataUpdated", recentData);
    }
}, 10000);

function processData(data: DataItem) {
    console.log("Beep Beep!");
    console.log("Processing Data: ", data);
    dataProcessor.process(data);
    console.log("Beep Beep! Brrrrr.... Done!");
}

When the task is completed a new PR is ready for review:

Diff showing lots of deletions and additions

Boom!

As a reviewer, I feel I have to do my best to verify all of this. That means first reading the commit message:

Add pre- and post-processing events

Now it's possible to listen to two new events:
- preDataProcess
- postDataProcess

Also, restructured the code a bit to make it easier to read.
Always happy to leave the code a bit better than before!

So yeah, sure, I can see two new lines that emit these events and they also include the data object. Looks good to me… but there is all of this other stuff going on. If I am going to put my stamp on it (LGTM), I will have to make sense of it.

The commit message says it is just restructuring of code, but is it really? Do we need a review process if I should just take the developers word for it? No, I will start matching the red with the green and try to figure out what has been done here.

Even though the above is just a tiny amount of code, there is enough changes that I will have to really focus. It is mentally taxing. Multiply, and it quickly escalates out of control.

The Solution⌗

Instead of me having to figure it all out on my own I would much prefer that the developer tells me a story in the form of commits and diffs:

Move processData function to before code execution

Diff showing clean movement of a function

Extract startPolling function for readability and reuse

Diff showing extraction of function. A bit much whitespace changes.

That one was a bit messy, but if I use the option to ignore whitespace changes:

Diff showing extraction of function with whitespace changes ignored

Move event listener down to code execution for readability

Diff showing code re-organized

Use constant POLLING_INTERVAL_IN_MS instead of hardcoded value

Diff showing introduction of constant in place of hardcoded value

Ah, so that’s what that number is…. errr, wait a minute… 10 000 –> 1 000?

At this point, I would add a code review comment and ask the developer if this was intentional or not. Did you mean to reduce the interval to 1000? If so I’d like the change to be it’s own commit with the details of why. Most likely though, the developer just didn’t want to wait 10 seconds after start when testing/debugging. A very common thing to do, and also a very common thing to forget afterwards.

Finally the last commit which was the main task:

Add pre- and post-processing events

Diff showing addition of two events being emitted

With a pull request on this format, I can quickly go through and verify the commits one at a time, and then continue with my own work.

The Takeaway⌗

We all accumulate noise while developing and it quickly becomes a huge burden for a code reviewer looking to give a good review.

Please think about that the next time you feel someone is nitpicking. Is is truly nitpicking, or is it a team member just really wanting to help out?

In my experience, spending some extra time on noise removal will pay back tenfold to the team in the long run.

PS: If I think a bit more about my code in the first place, maybe I won’t have to redo it later (slow and steady wins the race?).