loading...
Cover image for Clean code exercises - part 1

Clean code exercises - part 1

ederchrono profile image Eder Díaz Originally published at ederdiaz.dev ・3 min read

Illustration by Ivan Haidutski from Icons8

You've probably read and listen a lot about Clean Code and probably you're tired of acronyms like YAGNI, DRY and KISS. All of this usually goes directly to your brain's recycle bin because you don't practice it enough.

After years of reading other people's code in code reviews, I've developed an 'eye' to catch bad code, and I think you can develop it too by reading the scenarios I've designed.

The following examples aren't necessarily flawed code, I mean, imagine they don't have any bugs and they do the job, but the aren't as maintainable as they could be. Read each example, try to identify the problem and imagine what would you do to solve it.

Scenario 1

function canBuyBeer(age, money) {
  if(age >= 21 && money >= 20) {
    return true
  }
  return false
}
Enter fullscreen mode Exit fullscreen mode

What's the problem?

(don't read until you are done with above's code)

The problem in this example is the arbitrary numbers that are being used in the if statement. Because of the method context you might infer that 21 is the legal age to drink and 20 is the beer price, but this isn't too straight forward at first sight. These are usually called magic numbers.

Solution

One way to solve this is creating named constants for the numbers. This makes it easier to read.

function canBuyBeer(age, money) {
  const legalDrinkingAge = 21
  const beerPrice = 20
  if(age >= legalDrinkingAge && money >= beerPrice) {
    return true
  }
  return false
}
Enter fullscreen mode Exit fullscreen mode

Also if in the future something changes, like the beer price, it will be less prone to error by changing the constant value instead of finding and replacing the appearances of 20.

Scenario 2

function shouldShowImage(itemIndex, article, showAllImages) {
  return [0, 1, 2].includes(itemIndex)
    ? !!article.imageUrl
    : showAllImages
      ? !!article.imageUrl
      :false
}
Enter fullscreen mode Exit fullscreen mode

What's the problem?

(Remember to try to identify this by yourself first)
There's too many things happening in that return statement. The person that wrote this might thing is clever to use idiomatic features to solve things in a line or a couple of lines of code, that's why this is called clever code.

Solution

Just be explicit on what is the intended behavior and make it easy to read, even if that means splitting the code into more lines.

function shouldShowImage(itemIndex, article, showAllImages) {
  if(!article.imageUrl) {
    return false
  }
  if(showAllImages) {
    return true
  }
  const isItemOneTwoOrThree = [0,1,2].includes(itemIndex)
  if(isItemOneTwoOrThree) {
    return true
  }

  return false
}
Enter fullscreen mode Exit fullscreen mode

There were many refactoring steps between the example and the solution, but I assure you that both have the same behavior.

If a peer defends their clever code by saying that the solution is making the application become larger, that's likely not true, most modern languages when compiled or minified will be smaller than any clever code created by a human.

Scenario 3

function getArea(shape, width, height, radius) {
  if(shape === 'circle'){
    return Math.PI * radius * radius
  } else if(shape === 'square') {
    return width * width
  } else if(shape === 'rectangle') {
    return width * height
  }
}
Enter fullscreen mode Exit fullscreen mode

What's the problem?

This method has too many responsibilities. Whenever someone adds a new shape also a new if/else statement will need to be created. Also in this case the area calculations are one liners but if they become more complex, for example to validate the inputs, this method would become huge.

Solution

Separate them into strategies with their own method, this way if they grow in size they'll be easier to maintain.

const circleStrategy = (shape) => Math.PI * shape.radius * shape.radius

const squareStrategy = (shape) => shape.width * shape.width

const rectangleStrategy = (shape) => shape.width * shape.height

const areaStrategies = {
  circle: circleStrategy,
  square: squareStrategy,
  rectangle: rectangleStrategy
}

function getArea (shapeName, width, height, radius) {
  const shapeObject = { width, height, radius }
  const strategy = areaStrategies[shapeName]
  return strategy(shapeObject)
}
Enter fullscreen mode Exit fullscreen mode

Notice we also improved the mechanism to choose a strategy by using a dictionary instead of the multiple if/else statements.

Well, that's it for today's practice, I hope it helps you to get your spider sense tingle in the next code review you do.

I'll follow up soon with more examples in a future post, if you have ideas for more scenarios put them in the comments section.

Discussion

pic
Editor guide
Collapse
jonrandy profile image
Jon Randy

If, in a code interview, someone wrote code along the lines of:

function someFunction() {
  if (someBooleanExpression) {
    return true
  }
  return false
}
Enter fullscreen mode Exit fullscreen mode

I wouldn't hire them

Collapse
alexmario74 profile image
Mario Santini

I would like to know why?

I think that clean code is not the main concern on a whiteboard job interview.
But cleaning up some code can also be a good question for a candidate.

Collapse
jonrandy profile image
Jon Randy

The if and two returns are entirely superfluous. This type of code to me indicates a lack of understanding of what a boolean expression is. My above example should really be written:

function someFunction() {
  return someBooleanExpression
}
Enter fullscreen mode Exit fullscreen mode

Or, better still:

const someFunction = () => someBooleanExpression
Enter fullscreen mode Exit fullscreen mode

There is nothing tricky or clever going on in my code. Being overly verbose is not the same as being clean

Thread Thread
alexmario74 profile image
Mario Santini

I see.

I think that writing pseudo code on a whiteboard explaining what I have in mind, it's not the same as writing production code.

The uncecessary if and return statement will help exposing the implementation to another humans.

It's just that, I often refctor boolean expressions as you did here, so it is just that I was courious about your reasons about failing a job interview.

Thread Thread
jonrandy profile image
Jon Randy

The code should never be written that way in the first place - it's as bad as writing something like this in pseudocode:

if true return true else return false
Enter fullscreen mode Exit fullscreen mode

Definitely a red flag

Thread Thread
ogzhanolguncu profile image
Oğuzhan Olguncu

Also, violates SOLID's first principle, because now function
does two thing instead one.

Thread Thread
peerreynders profile image
peerreynders

The Single Responsibility Principle has nothing to do with "do just one thing" - that is a common misconception.
The Single Responsibility Principle:

Gather together those things that change for the same reason, and separate those things that change for different reasons.

So "those (many) things that change for the same reason" revolve around the same single responsibility.

Don't Repeat Yourself is another one - it' not about removing repetition or duplication:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

Sometimes duplication isn't about the same thing (The Wrong Abstraction):

prefer duplication over the wrong abstraction

Also: The SOLID Design Principles Deconstructed (2013)

Basics of the Unix Philosophy:

(i) Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new features. (Doug McIlroy 1978)

Collapse
darkwiiplayer profile image
DarkWiiPlayer
const circleStrategy = (shape) => Math.PI * shape.radius * shape.radius

const squareStrategy = (shape) => shape.width * shape.width

const rectangleStrategy = (shape) => shape.width * shape.height
Enter fullscreen mode Exit fullscreen mode

Those functions are poorly named. Proper names would be areaCircle, areaSquare and areaRectangle respectively. You can turn the names around, but putting area first makes for easier sorting.

Collapse
ederchrono profile image
Eder Díaz Author

Yeah I wasn't too careful with the naming here since that wasn't the problem of the exercise. I think that calculateAreaCircle would be better though, since methods should have a present tense verb.

Collapse
darkwiiplayer profile image
DarkWiiPlayer

since methods should have a present tense verb

I don't see any methods in that code.

Collapse
schmelto profile image
Tom Schmelzer

Nice and easy understanding. Thanks for sharing :)