Home > Code Review How To: Brevity and Repetition

Code Review How To: Brevity and Repetition

28 Feb 2022

16 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:

• Code organization
• Clarity and simplicity
• Brevity and repetition

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.

Technology vector created by starline - www.freepik.com

Today’s topic

Today’s topic is brevity and repetition: two closely related topics. Let’s start with a brief overview of what repetition is:

Repetition

One of the classic abbreviations you learn early on in your programming career is “DRY”: “don’t repeat yourself.” Don’t just write the same code over and over again: generalize it into reusable functions.

And indeed, in many respects, this is good advice! If you’ve got a piece of code that repeats in many places in your app, then things get tricky when you have to change it. Changing one piece of repeated code means changing every instance of that repeated code. Perfectly. Every single time. Taking care not to miss a single repetition. This is easy enough with find and replace, but what if some instances of repeated code need special treatment? Like a specific variable which this code needs to run, or maybe a different function call in that code which isn’t present elsewhere. In simpler scenarios, find and replace is a perfectly adequate tool, but in more nuanced scenarios it becomes a crude, indiscerning hacksaw, matching against and blowing away both the code that needs to change and the code which doesn’t need to change.

This is error prone to say the least. And tedious. And often exhausting. So to make our lives easier, we refactor repeated code into functions, abstracting many rote procedures performed at many points in an application into one tidy wrapper that holds it all. Make a change in one place, and that change will be reflected everywhere the function is called.

Simple! Right? Well, kind of.

There’s a reason that we still code this way even though we’ve got all kinds of wonderful tools to help us avoid repetition: abstraction is hard. Knowing what abstractions to apply to a given problem, recognizing commonly-occurring patterns, building in flexibility and refactorability to abstracted code: this takes experience. You’ve got to burn your fingers on the stove of knowledge a bunch of times before you acquire the nuanced understanding of how to wield the abstraction hammer appropriately.

It’s very much an art vs. science question - something that’s hard to teach and impossible to generalize into a set of hard and fast rules.

But generalize we must. So here is a set of fuzzy ideas about how and when to refactor and abstract:

• If you try to turn repetitive code into a reusable function too early, then it’s very easy to tie yourself into knots. When you’re in the early stages of writing an app, you’re an explorer. Unless you’re doing some rote coding exercise that you’ve done a hundred times before, you don’t really understand the problem you’re trying to solve with your code until you’ve spent some time working within it and exploring the nuanced facets and edge cases that it contains. Until the edges of the problem become better defined, many of the predictions you make about the kinds of functions you’ll need will be premature and inaccurate.
• Thinking about your initial explorations as a first draft is a good way to think of the process. If you’re coding blindly in the dark, you may as well hard code things, copy and paste code, and generally just hack around until things start working. Once the draft is fleshed out, the editing and refining process can begin. You can start to look for repetitions, similarities, variations on themes that can be abstracted into functions. You’ll have your hard-won knowledge of the problem domain to lean on, and you can more confidently identify which code would benefit from refactoring, and which code should be left alone.
• The refinement stage is also a great opportunity to step back and think about your code. It’s very easy to get wrapped up in the details when programming, which tends to wash out the big picture. If you take some time once in a while to look at the whole, then you’ll start to see and appreciate a lot of little things that can be improved or done differently.

Ultimately, repetitive code is a problem because of our limited human brains. At a small scale, repetition is a small problem. But as an app grows, the sheer volume of characters and words starts to become more and more expensive. You pay that cost with the time and mental effort you spend reading everything and cramming all of that information into your mind.

Brevity

Brevity is a closely related topic. If reducing repetition is the art of abstracting repeated code into flexible and reusable functions, then brevity is the art of communicating the concepts that underly variable names and procedures in the shortest terms possible. The art of expressing an idea with the fewest characters possible, but no fewer.

And just like repetition, brevity itself is a sword that cuts both ways:

• At its best, brevity helps you see more at a glance: if your code is short and to the point, then you can fit more of it on your screen, and you can therefore see more of the big picture at once.

• And yet, there is a line to draw with brevity as well. If you go too far and your code ends up too brief and terse, then it can become inscrutable. But even still, “too brief” is largely a matter of taste and experience. Would you rather read this:

    const f = x => x * x
    const g = x => x * -1
  
    const x = f(g(2)) 
    // -> 4
  

  or this

    const squareNumber = function(number) {
        return number * number
    }
    const numberToNegative = function(number) {
        return number * -1
    }
  
    const negativeTwoSquared = squareNumber(numberToNegative(2))
    // -> 4
  

When it comes to which is “better,” you could make a case for either of those examples. Which one you decide to go with is a personal matter. What are you comfortable and familiar with? What is your team comfortable and familiar with? Which one “feels” right? These are all important and valid questions when deciding where to draw the line.

Original code and example

The code we’re looking at today is the classic game of Pong. Here’s the original code below and a working example you can play (“W” and “S” control player 1, the up and down arrows control player two, press Enter to start).

Have a look at the code, play a couple rounds of the game. We’ll review the code for repetition and brevity in the next section.

Review

With our mission in mind, let’s start in on the code review. There are a few comments outside the scope of repetition and brevity, but all of it is intended to make the code easier to read and quicker to understand.

		@@ -0,0 +1,123 @@
		let gameState = 'start';
andyfry01 8 days ago There are a lot of hardcoded strings throughout this app: Game states: "start", "play" Keys that players have pressed to move the paddles or start the game: "w", "s","ArrowUp", "ArrowDown", "Enter" And even some hard-coded numbers: 0 and 1 are used to represent the four directions that the ball can move Strings are great to represent state concepts: they are clear and expressive indicators of what a state change signifies. I would argue that numbers are not as expressive as strings for communicating state changes. The usage of numbers for ball direction is kind of "boolean" in nature: 0 for "up", 1 for "down", but you have to study the code closely to understand what 0 actually means in the context in which it's being used. A string that clearly states "up" or "down" doesn't require close study to understand. So! We've got some good strategies for representing state change, and some strategies that would benefit from refactoring to strings, but we can do better still. Seeing as the strings are hard-coded, it makes it easier to misspell them, or forget to update all of the instances of a hardcoded string if you decide to refactor the string in the future. We can make our own home-rolled Typescript-style enums to save these strings in objects. This lets us divide the strings up into categories (states, movement keys, ball directions), and makes them easy and foolproof to update or expand if we need to: const MovementKeys = { P1_UP: "w", P1_DOWN: "s", P2_UP: "ArrowUp", P2_DOWN: "ArrowDown", }; const Keys = { ENTER: "Enter", ...MovementKeys, }; const GameStates = { START: "start", PLAY: "play", }; const BallDirections = { UP: "up", DOWN: "down", LEFT: "left", RIGHT: "right", }; // usage examples: let gameState = GameStates.START; if (e.key == MovementKeys.P1_UP) { // move p1 paddle up }
		let paddle_1 = document.querySelector('.paddle_1');
		let paddle_2 = document.querySelector('.paddle_2');
		let board = document.querySelector('.board');
		let initial_ball = document.querySelector('.ball');
		let ball = document.querySelector('.ball');
		let score_1 = document.querySelector('.player_1_score');
		let score_2 = document.querySelector('.player_2_score');
		let message = document.querySelector('.message');
		let paddle_1_coord = paddle_1.getBoundingClientRect();
		let paddle_2_coord = paddle_2.getBoundingClientRect();
		let initial_ball_coord = ball.getBoundingClientRect();
		let ball_coord = initial_ball_coord;
		let board_coord = board.getBoundingClientRect();
		let paddle_common =
		document.querySelector('.paddle').getBoundingClientRect();
andyfry01 8 days ago I like that all of your setup code is organized at the top of the file. By putting you DOM element selectors and state in one place, it makes it much easier to navigate between what your app is acting on and the functions that perform the actions. However, you've got game state, DOM selectors, and game constants all mixed together in one heap. You can make your code even easier to understand for outsiders by organizing them into their own separate categories, separated by newlines. Additionally, many things which are effectively immutable (your DOM elements, the bounding rects of the board and paddle) are defined with let variables. let implies that a variable will change over the runtime of a program, and const signifies that it won't. If you define your immutable variables with const and your mutable variables with let, you can better communicate to the reader what is static and what is fluid. // DOM elements const paddle_1 = document.querySelector(".paddle_1"); const paddle_2 = document.querySelector(".paddle_2"); const board = document.querySelector(".board"); const initial_ball = document.querySelector(".ball"); const ball = document.querySelector(".ball"); const score_1 = document.querySelector(".player_1_score"); const score_2 = document.querySelector(".player_2_score"); const message = document.querySelector(".message"); const paddle_common = document.querySelector(".paddle").getBoundingClientRect(); // constants const board_coord = board.getBoundingClientRect(); const initial_ball_coord = ball.getBoundingClientRect(); // state let gameState = GameStates.START; let ball_coord = initial_ball_coord; let paddle_1_coord = paddle_1.getBoundingClientRect(); let paddle_2_coord = paddle_2.getBoundingClientRect(); ``
		let dx = Math.floor(Math.random() * 4) + 3;
andyfry01 8 days ago These random numbers you're generating are used repeatedly in numerous places: lines 18-21, 30-33, 88-89, and 97-98 all use the same random number generator functions. If you wrap this repetitive code up in a function, you can: give the function a name (like randomCoordinate or randomDirection), which will help to express what those random numbers represent, and will aid comprehension for others who are reading your code. give yourself an easy way to update all instances of those random numbers. Maybe in the future you decide that you don't want to round down, or maybe you want to increase or decrease what you're multiplying or adding to your random coordinates to add more variation to ball placement between rounds. As it is, you'd have to update all of your random number functions in many places, and you could miss some instances if you're not diligent. If those parameters are wrapped up in a function, however, then you only have to update them in one place and you're done! const randomCoord = () => Math.floor(Math.random() * 4) + 3; const randomDirection = () => Math.floor(Math.random() * 2); let dx = randomCoord(); let dy = randomCoord(); let dxd = randomDirection(); let dyd = randomDirection(); // etc
		let dy = Math.floor(Math.random() * 4) + 3;
		let dxd = Math.floor(Math.random() * 2);
		let dyd = Math.floor(Math.random() * 2);
		document.addEventListener('keydown', (e) => {
andyfry01 8 days ago The nature of the keydown event listener we're working with here has divided the code within into two nice parts: code that starts the game, and code that moves the paddle. Including both pieces as part of one big callback, however, makes things just a little bit harder to navigate. If we take these individual if blocks and break them up into functions, we can simultaneously give names to what's being done and make it easier for future readers to ignore what they're not interested in. Need to know about the mechanics for how the rounds are started? Look at startRound. Need to change how the paddles move? Look in movePaddle. const handleKeyPress = (e) => { if (e.key === Keys.ENTER) { startRound(); } if (gameState === GameStates.PLAY && Object.values(MovementKeys).includes(e.key)) { movePaddle(e) } };
		if (e.key == 'Enter') {
		gameState = gameState == 'start' ? 'play' : 'start';
		if (gameState == 'play') {
		message.innerHTML = 'Game Started';
		message.style.left = 42 + 'vw';
		requestAnimationFrame(() => {
		dx = Math.floor(Math.random() * 4) + 3;
		dy = Math.floor(Math.random() * 4) + 3;
		dxd = Math.floor(Math.random() * 2);
		dyd = Math.floor(Math.random() * 2);
		moveBall(dx, dy, dxd, dyd);
		});
		}
		}
		if (gameState == 'play') {
		if (e.key == 'w') {
andyfry01 8 days ago There's a lot of repeated code in here for moving paddles up and down, with only slight variations between the various implementations (which paddle we're moving, which direction, etc.). To make this code a little shorter, and to make it easier to understand at a glance, I'd suggest abstracting all this into movePaddleDown and movePaddleUp functions. That, combined with a little bit of thoughtful variable naming and some nice ES6 string-template code, can make this a lot easier to grok and work with: // an example implementation for movePaddleUp // movePaddleDown would be the same thing, but with all the math appropriately flipped around const movePaddleUp = (paddle) => { const nextPosition = Math.max( board_coord.top, paddle.top - window.innerHeight * movementInterval ); return `${nextPosition}px`; }; // Example function calls: if (e.key == MovementKeys.P1_UP) { paddle_1.style.top = movePaddleUp(paddle_1_coord); } if (e.key == MovementKeys.P1_DOWN) { paddle_1.style.top = movePaddleDown(paddle_1_coord); }
		paddle_1.style.top =
		Math.max(
		board_coord.top,
		paddle_1_coord.top - window.innerHeight * 0.06
		) + 'px';
		paddle_1_coord = paddle_1.getBoundingClientRect();
		}
		if (e.key == 's') {
		paddle_1.style.top =
		Math.min(
		board_coord.bottom - paddle_common.height,
		paddle_1_coord.top + window.innerHeight * 0.06
		) + 'px';
		paddle_1_coord = paddle_1.getBoundingClientRect();
		}
		if (e.key == 'ArrowUp') {
		paddle_2.style.top =
		Math.max(
		board_coord.top,
		paddle_2_coord.top - window.innerHeight * 0.1
andyfry01 8 days ago This is a prime example of how hard-coded values used repetitively across a codebase can quietly introduce what I will call "unintended functionality". Nothing is "broken" per se, so you couldn't properly call it a "bug" in the sense that most people would understand that term, but I don't think you intended to give player two such a huge structural advantage over player one. Player two's paddle moves almost twice as fast as player one's paddle (0.1px per keystroke for player two, 0.06px for player one). If this movement interval had originally been coded as a constant, with the value living and being updated from a single point, then this wouldn't have made it into the final version.
		) + 'px';
		paddle_2_coord = paddle_2.getBoundingClientRect();
		}
		if (e.key == 'ArrowDown') {
		paddle_2.style.top =
		Math.min(
		board_coord.bottom - paddle_common.height,
		paddle_2_coord.top + window.innerHeight * 0.1
		) + 'px';
		paddle_2_coord = paddle_2.getBoundingClientRect();
		}
		}
		});
		function moveBall(dx, dy, dxd, dyd) {
		if (ball_coord.top <= board_coord.top) {
		dyd = 1;
		}
		if (ball_coord.bottom >= board_coord.bottom) {
		dyd = 0;
		}
		if (
		ball_coord.left <= paddle_1_coord.right &&
		ball_coord.top >= paddle_1_coord.top &&
		ball_coord.bottom <= paddle_1_coord.bottom
		) {
		dxd = 1;
		dx = Math.floor(Math.random() * 4) + 3;
		dy = Math.floor(Math.random() * 4) + 3;
		}
		if (
		ball_coord.right >= paddle_2_coord.left &&
		ball_coord.top >= paddle_2_coord.top &&
		ball_coord.bottom <= paddle_2_coord.bottom
		) {
		dxd = 0;
		dx = Math.floor(Math.random() * 4) + 3;
		dy = Math.floor(Math.random() * 4) + 3;
		}
		if (
		ball_coord.left <= board_coord.left ||
		ball_coord.right >= board_coord.right
		) {
		if (ball_coord.left <= board_coord.left) {
		score_2.innerHTML = +score_2.innerHTML + 1;
		} else {
		score_1.innerHTML = +score_1.innerHTML + 1;
		}
		gameState = 'start';
		ball_coord = initial_ball_coord;
		ball.style = initial_ball.style;
		message.innerHTML = 'Press Enter to Play Pong';
		message.style.left = 38 + 'vw';
		return;
		}
		ball.style.top = ball_coord.top + dy * (dyd == 0 ? -1 : 1) + 'px';
andyfry01 8 days ago There's a lot of stuff going on in here: Math to determine the next top and left position of the ball Logic to determine whether the ball should move up, down, left or right on the next frame Coercion of a number value to a string by way of adding a number to a string literal Setting the measurement unit for the value derived by all of the above (px) And finally, setting the actual property of the ball's DOM object to the final string so we can move the ball It is terse, which is one dimension by which you can measure the readability of code, but I'd also call it opaque in meaning and in function, which makes it less readable. A lot of assumed knowledge of what these variables mean and experience with the weird, hairy corners of JS – specifically how the language handles things like combining numbers with strings using the + operator – is required to grasp the functionality in full. Using a couple variable names, some of the constants we mentioned in another comment, and ES6 string templating, we can make this a lot more understandable while only making the code a little bit longer: const nextTopCoord = ball_coord.top + dy * (dyd === BallDirections.UP ? -1 : 1); ball.style.top = `${nextTopCoord}px`; // same thing for the next left coordinate
		ball.style.left = ball_coord.left + dx * (dxd == 0 ? -1 : 1) + 'px';
		ball_coord = ball.getBoundingClientRect();
		requestAnimationFrame(() => {
		moveBall(dx, dy, dxd, dyd);
		});
		}

Refactored code and example

Now that we’ve done the review, let’s check out the code with the changes implemented:

Conclusion

Thank you for reading! In this blog, we:

• Defined what brevity and repetition are, along the costs of verbose and repetitive code
• Took a look at some example code, and reviewed it with the objective of making it clearer to read by making it less repetitive and verbose
• Implemented those changes for a “before and after” perspective

Thank you for reading! Feel free to leave a comment below or to reach out to me if you have any other thoughts about anything we’ve covered today, I’d love to hear from you!

- Andy

Leave a comment

Name E-mail Website (optional)

Comment

Related Posts

• Some guy naively bungles through his first steps in Elixir
  20 Apr 2022
• Map from Scratch
  23 Mar 2022
• Code Review How To: Clarity and Simplicity
  01 Mar 2022