Blog for Shendy

Practical Code Smells - Sample 1

Practical Code Smells - Sample 1

Ahmed Shendy's photo
Ahmed Shendy
·

3 min read

Take a look a this code, share in comments what are your thoughts/notes about it?

struct Bottles {
    func song() -> String {
        verses(from: 99, downTo: 0)
    }

    func verses(from high: Int, downTo low: Int) -> String {
        (low...high)
            .reversed()
            .map { verse($0) }
            .joined(separator: "\n")
    }

    func verse(_ n: Int) -> String {
        return "\(n == 0 ? "No more" : "\(n)") bottle\(n != 1 ? "s" : "")" +
            " of beer on the wall, " +
            "\(n == 0 ? "no more" : "\(n)") bottle\(n != 1 ? "s" : "") of beer.\n" +
            "\(n > 0 ? "Take \(n > 1 ? "one" : "it") down and pass it around" : "Go to the store and buy some more"), " +
            "\(n - 1 < 0 ? "\(99)" : n - 1 == 0 ? "no more" : "\(n - 1)") bottle\(n - 1 != 1 ? "s" : "")" +
            " of beer on the wall.\n"
    }
}

Are you done? Now, let me share the thoughts I learned about issues in this code.

This code looks clever, it performs a neat trick. It is very concise, while simultaneously retaining loads of duplication.

You find it hard to understand, because it is inconsistent, duplicative, and it contains hidden concepts that it doesn't name.

You can find different styles of conditionals, here:

\(n == 0 ? "No more" : "\(n)")

and here, where this lines uses ternary within ternary:

n - 1 < 0 ? "\(99)" : n - 1 == 0 ? "no more" : "\(n - 1)") bottle\(n - 1 != 1 ? "s" : ""

This inconsistent styling of conditionals makes it hard for human to parse; it raises costs without providing benefits.

The code duplicates both the data and logic.

String duplication like "of beer" and "on the wall" is easy to see and understand, but logic is really hard, and putting such duplicated logic inside strings makes painful confusion for the reader.

The logic for "bottle" pluralization is done in three places.

bottle\(n != 1 ? "s" : "") // in two lines

bottle\(n - 1 != 1 ? "s" : "") // in one line

The need to sometimes say "bottle" and other times say "bottles" means something, and the need to sometimes use n and other times use n-1 means something else. We are left to figure out these hidden concepts, because the code shows no clue about what these meanings might be.

The string inside verse method, contains embedded logic. Each logic serves some purpose, and it should be moved into methods, then verse fill itself with values by calling them.

The issue is to create a method you need two things:

  • identifying the code you would like to extract

  • deciding on method name

and naming things are just plain hard. This code makes it hard to create methods, it combines many logic, which also mixed together, into a small section of code which makes it difficult to isolate and name any single concept.

Code is easy to understand when it clearly reflects the problem it is solving, and this code does not. It looks like it was difficult to write, and will be a challenge to change.

End of post

code smellSwift