Code Review How To: Clarity and Simplicity

01 Mar 2022

15 minute read

Welcome to Code Review How To! This is a three-part series where we take a look at the art and science of performing code reviews. In this series, we’ll look at some JavaScript code examples pulled from a few very cool blog posts and Codepens. We’ll examine the code through three different lenses:

There are a lot of ways you can review and critique code: performance, security, idiomaticity, the list goes on. In my own practice, I like to focus on readability, that elusive white deer in the forest which makes code easy to understand at a glance without getting all tangled up in the specifics.

Code that’s easier to read is easier to write, and easier to change and adapt to new needs as they arise. My grand ambition in reviewing code is to bring it just a little closer to the Golden Dream of programmers everywhere: code that says what it means, is easy to modify, and holds no surprises.

What constitutes “easier to read” is, of course, largely up to interpretation. I said “art and science” in that first paragraph because the craft of programming is in many ways still in its infancy, and much of what we do is driven by taste and convention rather than by “hard universal truths” that other professions like mechanical engineering enjoy. There are some things in the programming world which can be said to be “true” in that engineering sense, but much else is still theory.

There are a few things, however, which look like proto-truths to me. Things like immutability, functional style, and some basic tenets of organization and reuse, which I think can be applied to great effect in many different contexts. If you’re down to hear my perspective on the matter, I’d love to bring you along with me as we explore these concepts.

Title graphic, background of ones and zeroes with 'Code Review How To: Clarity and Simplicity' overlayed on top

Technology vector created by starline -

Today’s topic

Today’s topic is making things simple and clear.

If you’ve spent any time programming at all, you know that things can get out of hand very, very quickly when it comes to code complexity. Functions which originally expressed simple, easy-to-understand bundles of operations become long and inscrutable. Your variable names get longer and weirder as you run out of words to describe things. The small beginnings of a solution to a problem quickly grow into massive and incomprehensible piles of spaghetti.

The whole thing slowly morphs from a sunny, peaceful meadow full of beautiful flowers into a dark, scary forest of unintended consequences and bad surprises.

It can be discouraging to watch this happen, and it can be discouraging to try to clean messy code up too. The strands of spaghetti can get really hard to untangle! But despair not dear readers, because we have tools and techniques to tame complicated and murky code! And code review time is a great opportunity to make use of those tools, so let’s dive in.

Broad themes

Defining a one-size-fits-all tool set to tackle every problem is of course impossible, but there are some general guidelines that you can consider when hacking through the underbrush:

Let’s look at these individually:

Break large functions into smaller functions

A classic symptom of complexity is a function that does too much. A function that spans hundreds of lines, with code blocks and variable names that disappear off of the right edge of the screen. A huge list of parameters. A name that bears little or no resemblance to what it actually does. You’ve seen it, I’ve seen it, we’ve both written stuff like this.

Many of these functions start out innocently enough, with a clear name and containing a basic and comprehensible set of tightly-related functionality, but over time things start to creep in. A few edge cases here, a couple of new requirements there, and all of a sudden you’ve got a huge Swiss Army blob on your hands that does everything and can’t be understood.

Cutting up these mega-functions into smaller bits can be intimidating, so start small. Forget the modules, namespaces, and capital A Architecture considerations: it can be as simple as wrapping up a piece of related functionality into a closure within your blob function. As you do this more and more, patterns will start to emerge, a sensible organization will become clear, and you can start to migrate these functions out when the time is right.

But how small is too small? A good rule of thumb is to write functions that do “one thing.” Now, that “one thing” could be as small as formatting a string and could be as big as initializing your whole app, but each function should contain one set of related functionality. As the great Ron Swanson put it:

Dont’ half-ass two things, whole-ass one thing.

Getting to the point with variable names

Somewhere in the desert, inscribed on an ancient tablet at the foot of a crumbling statue, is written this joke:

There are only two hard things in computer science: cache invalidation, naming things, and off-by-one errors.

Another way to increase clarity is to focus on making good names for functions and variables.

I know, I know, this is one of the most obvious things I could possibly say. The things you could know about good variable naming is a vast and deep ocean, but there are some easy tips we can fish out of it.

One of those tips is the classic Strunk and White writing tip: omit needless words. If you overload a variable with too much context, qualifiers, and other hangers-on, you get what Kevlin Henney in the Seven Ineffective Coding Habits of Many Programmers calls “homeopathic naming”: a watering down of meaning until there’s nothing left.

Would you rather read this:

Or this:

Another name for this is Hungarian Notation. This is a style of variable naming where you include words to indicate a variable’s intention or type. Think things like userStatusString or canEditBoolean. There are advantages to this style of variable naming. Practiced in moderation, it can serve you well, but more often than not it just leads to long names which are harder to understand than they should be.

Using language tools to add intent and expressivity

Why do programming languages exist? If it all boils down to ones and zeroes in the end, why don’t we just type those out directly and get rid of the middle man? Why do we have so much diversity in the ways that we program computers?

Well, it’s possible to type out the ones and zeroes by hand, but it’s way easier for us to work with human-understandable abstractions between us and the bare metal. Something that translates our very non-computerish ways of thinking into computerish terms.

And so, if we can agree that more human-ish abstractions help us to think and understand what we’re working with, then there must exist better abstractions and worse abstractions: symbolic representations of common operations that can be compared and ranked against each other in terms of how clearly they explain their functioning.

One way to determine whether an abstraction is better or worse than another is whether you can tell what’s going on from the outside; i.e., whether you can skim over a function and accurately imagine what it does without having to read every single line.

It’s like a book in a way. Imagine if we never transitioned from rolled-up scrolls and parchment to sheets of paper bound in books. Both scrolls and books can store the exact same information, but a book is way easier to use than a scroll, and much of that ease-of-use comes from the very format of the book itself.

If you want to know what’s inside a parchment, you’ve gotta take it down from the shelf, unroll it and peek inside. Gently please! This thing is old as hell, and nobody’s opened it in like 200 years! And then to actually read the thing, you’ve gotta unroll it. If it’s small enough maybe you could do it in your lap, but larger and more officious scrolls need a flat surface to keep things from getting out of hand. If you’re looking for information at the bottom of the scroll, I’m sorry to say it, but you’ve gotta unroll and re-roll the whole thing to get all the way down there.

A book, on the other hand, has the title and the author on the front cover AND the spine, and it has summary information on the back sometimes too! You can get a great idea of what’s inside based on that alone, and you didn’t have to read a single page to find out. Maybe you didn’t even need to take it down off of the shelf. If it IS the book you’re looking for, it probably has page numbers and a table of contents at a minimum, and maybe an index, which makes it all the easier to flip to the exact point in the book that you need to go to, and easier still to flip back and forth between sections. No rolling and unrolling, just a flick of the thumb will do.

There’s a reason we don’t record our important information on scrolls anymore: technology advancements and a sprinkling of printing conventions have rendered them obsolete, and have made everybody’s lives a lot easier. And we can achieve similar results in our programs too. If we reach for better and more expressive abstractions, then everybody can understand each other’s code faster, with just a little training in the meaning and use of those abstractions.

Original code and example

The code we’re looking at today is a tic-tac-toe game.

The original can be found here. Play around with the code a little bit!


There are some things that I really like about the code, which are mentioned in the review. But there are a few things which could stand to be improved, specifially about getting to the point with variable names, utilizing abstractions to aid comprehension, and general organization. Here’s the reivew below:

@@ -0,0 +1,31 @@
<!doctype html>
<html lang="en">
<meta charset="UTF-8">
<meta name="viewport"
content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<title>Tic Tac Toe</title>
<link rel="stylesheet" href="style.css">
<h1 class="game--title">Tic Tac Toe</h1>
<div class="game--container">
<div data-cell-index="0" class="cell"></div>

andyfry01 8 days ago

By using div elements, this makes the game inaccessible for screenreader and keyboard users.

Fortunately, in order to remedy this, all you have to do is refactor these into button elements.

Buttons typically have a lot of browser-specific styling associated with them, which drives this tendency to use an unstyled div as a clean slate for designing a custom UI element. However, you can use a CSS button reset to mitigate this and get your buttons looking the way that you want them to. CSS Tricks has a great blog on the subject.

<div data-cell-index="1" class="cell"></div>
<div data-cell-index="2" class="cell"></div>
<div data-cell-index="3" class="cell"></div>
<div data-cell-index="4" class="cell"></div>
<div data-cell-index="5" class="cell"></div>
<div data-cell-index="6" class="cell"></div>
<div data-cell-index="7" class="cell"></div>
<div data-cell-index="8" class="cell"></div>
<h2 class="game--status"></h2>
<button class="game--restart">Restart Game</button>

<script src="script.js"></script>
<!DOCTYPE html>
@@ -0,0 +1,87 @@
const statusDisplay = document.querySelector('.game--status');

let gameActive = true;
let currentPlayer = "X";
let gameState = ["", "", "", "", "", "", "", "", ""];

andyfry01 8 days ago

Instead of a "flat" list of empty strings representing coordinates on the board, why not use some formatting to shape them to look like the board itself?

A couple of strategic newlines and tabs will really aid comprehension:

let gameState = ["", "", "", 
             "", "", "", 
             "", "", ""];

const winningMessage = () => `Player ${currentPlayer} has won!`;

andyfry01 8 days ago

Excellent practice to wrap things like this up into functions instead of hardcoded strings.

const drawMessage = () => `Game ended in a draw!`;
const currentPlayerTurn = () => `It's ${currentPlayer}'s turn`;

statusDisplay.innerHTML = currentPlayerTurn();

const winningConditions = [
[0, 1, 2],
[3, 4, 5],
[6, 7, 8],
[0, 3, 6],
[1, 4, 7],
[2, 5, 8],
[0, 4, 8],
[2, 4, 6]
Comment on lines +13 to +22

andyfry01 3 minutes ago

Really like how you've implemented the winning conditions for this as a static set of triples representing a winning board. It's a very data-driven approach to checking for a win.

Checking for a winning condition could be implemented programmatically, and you could also write a function which generates this array of triples programmatically as well. If the domain we were working in was bigger, that might be a better choice, but seeing as we're working in a small context with this game, I think a hard-coded set of winning conditions is the perfect match for the task at hand 👍

function handleCellPlayed(clickedCell, clickedCellIndex) {
gameState[clickedCellIndex] = currentPlayer;
clickedCell.innerHTML = currentPlayer;

function handlePlayerChange() {
currentPlayer = currentPlayer === "X" ? "O" : "X";
statusDisplay.innerHTML = currentPlayerTurn();

function handleResultValidation() {
let roundWon = false;
for (let i = 0; i <= 7; i++) {
const winCondition = winningConditions[i];
let a = gameState[winCondition[0]];
let b = gameState[winCondition[1]];
let c = gameState[winCondition[2]];
if (a === '' || b === '' || c === '') {
if (a === b && b === c) {
roundWon = true;

if (roundWon) {
statusDisplay.innerHTML = winningMessage();
gameActive = false;

let roundDraw = !gameState.includes("");
if (roundDraw) {
statusDisplay.innerHTML = drawMessage();
gameActive = false;

Comment on lines +34 to +64

andyfry01 3 minutes ago

Firstly, some things I really like about this code:

  • It's wrapped up in a discrete function with a good name
  • It leverages early returns, and doesn't tangle its logic in a bunch of if/else trees, which can be hard to refactor down the line
  • It has nice whitespace to separate out the different parts, which helps us focus on the individual pieces of the function.

There are some things we can do which will make it even more readable:

  • Refactoring your for loop to a reduce function. for loops are good for looping tasks which require mutating state. In this context, we're checking the current board against the game's winning conditions to see whether to continue the game or whether a win or draw has occurred. This is a "data-driven" activity, and reduce is a tool which is much better suited for data-driven tasks.
  • Refactoring some boolean logic to use strings which represent game states.
  • Giving names to the logic checks you're performing, which will help us to more easily understand what exactly constitutes a win vs. a draw. When you've got variables to represent concepts, it makes things easier to read, seeing as you can rely on the variable name to communicate what's going on. Without a variable name, you have have to read the logic itself to understand what's going on. The logic in this context is not that hard to understand, but it nevertheless takes more time and effort to grok than a named variable would.

So, that being said, here's how I would refactor the code:

// At the top of the file, we can define some handy constants which represent game states: 
const IN_PROGRESS = "in progress"
const WIN = "win"
const DRAW = "draw"

// the refactored function itself
function handleResultValidation() {
const gameStatus = winningConditions.reduce((status, winCondition) => {
    const a = gameState[winCondition[0]]
    const b = gameState[winCondition[1]]
    const c = gameState[winCondition[2]]
    // a "vector" means either a column or a row
    const incompleteVector = (a === '' && b === '' && c === '')
    const filledVector = (a === b && b === c)
    const boardFilledUp = !gameState.includes("") 
    const gameFinished = filledVector && !incompleteVector

    if (status === WIN) {
        return WIN

    if (boardFilledUp || status === DRAW) {
        return DRAW
    return gameFinished ? WIN : IN_PROGRESS;

return gameStatus === IN_PROGRESS ? handlePlayerChange() : endGame(gameStatus)

function handleCellClick(clickedCellEvent) {
const clickedCell =;
const clickedCellIndex = parseInt(clickedCell.getAttribute('data-cell-index'));

andyfry01 8 days ago

This is an example of a variable name that I'd consider too verbose.

We're inside a function called handleCellClick, which takes as an argument an object called clickedCellEvent, which we're grabbing a clickedCell value from. There's a ton of context already for what we're working with – a cell that's been clicked on – so clickedCellIndex, while admirable in that the name strives to clearly communicate the concept that the index belongs to, is a bit excessive.

You could argue that index would be enough to communicate what's going on here, or maybe cellIndex is a happy medium between too verbose and not verbose enough.

if (gameState[clickedCellIndex] !== "" || !gameActive) {

handleCellPlayed(clickedCell, clickedCellIndex);

function handleRestartGame() {
gameActive = true;
currentPlayer = "X";
gameState = ["", "", "", "", "", "", "", "", ""];
statusDisplay.innerHTML = currentPlayerTurn();
document.querySelectorAll('.cell').forEach(cell => cell.innerHTML = "");

document.querySelectorAll('.cell').forEach(cell => cell.addEventListener('click', handleCellClick));
document.querySelector('.game--restart').addEventListener('click', handleRestartGame);
Comment on lines +83 to +87

andyfry01 3 minutes ago

Things like query selectors for DOM elements fall into a category I refer to as "setup" or "bootstrapping" code. It's often helpful to place query selectors in their own section for organization, and to reference them by variable name instead of querying them directly inside a function. This aids in organization and separation of concerns between the setup for your app and the logic code which actually runs the functionality.

// Somewhere close to the top of the file, with the rest of your queried DOM elements: 
const cells = document.querySelectorAll('.cell')

// what you'd invoke on this line: 
cells.forEach(cell => cell.innerHTML = "");

Refactored code and example

And now that we’ve reviewed the code, let’s see what those changes look like in practice:


Thank you for reading! In summary, we:

If you have some thoughts to add, please add a comment below or reach out to me! I’d love to hear from you.

- Andy

Leave a comment

Related Posts