Is this a good code?

#code #review #codequality #programming

Disclaimers

First let me make two disclaimers:

I'm not trying to tell here that the developer that wrote this code is a bad developer. For all I know he/she might be a much better developer than me.
I write a bad code all the time, so this post is nor to try compare my work to their's.

The code

You probably noticed this famous tweet:

You can find the code in question here:
https://github.com/MinBZK/woo-besluit-broncode-digid-app/blob/ad2737c4a039d5ca76633b81e9d4f3f9370549e4/Source/DigiD.iOS/Services/NFCService.cs#L182-L206

The internet became split between two types of developers. There were some developers saying that this is a bad code and providing an interesting one-liners that can do the same thing, and there were others that were praising it for its readability and simplicity.

I don't think that this code is really bad, but I don't agree with those that say that this is an amazing code either. Although, I completely agree that it's highly readable and you clearly understand what the author was trying to do, I do think it has some flaws. But first, let me share my solutions to the same problem.

My version

The first thing I asked myself is how I would solve this problem. I came up with initial version:

which was criticised fairly for being less optimised than the original:

So I went on and made a second version:

that seems to be as readable and as efficient as original one:

So what is wrong with the original code?

For me a part from readability and efficiency there is another important criteria for a good code: maintainability.

Imagine a scenario where you've shipped the initial solution and you received right away the new product requirements that change its behaviour.

In this scenario, the less effort you have to make to adapt to these requirements, the more maintainable your solution will be.

And I can think of at least two potential changes to product requirements:

making the scale bigger / smaller (instead of 10 slots show 5 or 20)
change the symbol from blue dot to let's say a star.

The first case would require making the original code twice as long / short. Or in the worst case it might be a complete rewrite. And the second case would require a search and replace through the whole function body. (it might not be a big deal in this case but it just shows the flaw)

In my version the modifications for those new requirements would be the following:

extract the number of steps into a scale variable:

public static string GetPercentageRounds(double percentage) {
  const string blueSpotSymbol = "🔵";
  const string emptySpotSymbol = "⚪";
  const int scale = 20;
  // 0 to 10
  double blueSpotsToDraw = Math.Ceiling(percentage * 10) * scale/10;
  string overallProgress = "";

  for (int i = 0; i < scale; i++) {
    overallProgress += blueSpotsToDraw-- > 0 ?
      blueSpotSymbol :
      emptySpotSymbol;
  }

  return overallProgress;
}

just change the symbols at the top of the function

Even if there is another team that comes to you and asks you to make this code configurable and share it with them it is fairly easy to do. You can simply add scale and symbols to function parameters.

Conclusion

Maintainability is as important as efficiency or readability. The more maintainable we make the system today, the less work and technical debt we will leave to someone else in the future.