Recently, the Dutch government open-sourced the iOS application for their "DigiD" authentication service. A tweet with a snippet of that source code, presumably making fun of it, blew up into a debate about whether mocking it is even justified. The amount of debate over such a simple snippet of code highlights, in my mind, just how tricky software development can be.
Here is the code in question, written in C#:
private static string GetPercentageRounds(double percentage)
{
if (percentage == 0)
return "⚪⚪⚪⚪⚪⚪⚪⚪⚪⚪";
if (percentage > 0.0 && percentage <= 0.1)
return "🔵⚪⚪⚪⚪⚪⚪⚪⚪⚪";
if (percentage > 0.1 && percentage <= 0.2)
return "🔵🔵⚪⚪⚪⚪⚪⚪⚪⚪";
if (percentage > 0.2 && percentage <= 0.3)
return "🔵🔵🔵⚪⚪⚪⚪⚪⚪⚪";
if (percentage > 0.3 && percentage <= 0.4)
return "🔵🔵🔵🔵⚪⚪⚪⚪⚪⚪";
if (percentage > 0.4 && percentage <= 0.5)
return "🔵🔵🔵🔵🔵⚪⚪⚪⚪⚪";
if (percentage > 0.5 && percentage <= 0.6)
return "🔵🔵🔵🔵🔵🔵⚪⚪⚪⚪";
if (percentage > 0.6 && percentage <= 0.7)
return "🔵🔵🔵🔵🔵🔵🔵⚪⚪⚪";
if (percentage > 0.7 && percentage <= 0.8)
return "🔵🔵🔵🔵🔵🔵🔵🔵⚪⚪";
if (percentage > 0.8 && percentage <= 0.9)
return "🔵🔵🔵🔵🔵🔵🔵🔵🔵⚪";
return "🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵";
}
Let's ignore the lack of parameter validation (namely of ensuring that percentage is not NaN, negative, or infinity), since it would be the same regardless of solution, and it's not uncommon to omit that sort of rigorous error handling in a private method.
Aside from that, well... it omits curly brackets in the if blocks, which is a style thing many people dislike. It's very repetitious. It has unnecessary greater-than conditions (since each prior if's less-than-equal condition guarantees it). It does several more if comparisons than it technically needs to. In fact, given that the output is linearly related to the input, this can be a one-liner with just a little bit of math; let's call it the "math solution".
private static string GetPercentageRounds(double percentage)
{
int stage = (int)Math.Ceiling(percentage * 10);
return new string('🔵', stage) + new string('⚪', 10 - stage);
}
Repetition avoided; 21 lines reduced to 2. Problem solved...
...although if I were come back to that code in a month's time, I'd definitely have to stare at it longer to figure out what it does...
...and I'm not 100% confident that the multiplication won't cause floating point edge cases...
...and I guess it's less flexible now (e.g. to adjust the color of individual dots by stage)...
...and it's doing three string allocations now instead of returning a constant, so maybe that has performance implications...
...hmm. Maybe we need to remind ourselves of what "good" code is.
Question: What is Good Code? Answer: Code That Makes The Right Tradeoffs.
Good code has a number of qualities most developers would agree on:
- Arguably the most important, good code is readable. Code is written once but it is read many times, and understanding code is key to maintaining and extending it, so a solution that is easy to read and understand is usually better than one that is more clever.
- Good code is correct; it does what it's supposed to do. OK, maybe this is more important than readability, but then again, it's easy to fix a bug in readable code.
- Good code is performant; it should not needlessly waste system resources.
- Good code is easy to write, since productivity is important.
Often, even usually, some of these qualities are at odds with one another. Making code more readable often means not choosing the most performant possible solution, and similarly, optimizing a solution often makes it less readable. Code that's easy to write is often not readable, performant or correct.
What this means is that what makes for "good" code always depends on the circumstances of that code: its purpose, the code surrounding it, the current and future priorities of the end client or product manager, etc. We don't even know all those circumstances at the time the code is written, so we're doomed to never truly know what the best solution is, and just come up with the best we can.
With that in mind, let's have another go at comparing the qualities of these two solutions, along with yet another one added to the mix.
Comparing Solutions
The "original" solution is understandable at a glance, it doesn't use any fancy features a junior developer might not understand, it doesn't do any dynamic string allocations, and it's flexible for a few types of future changes (e.g. changing dot colors per stage or per position). On the other hand, it's many lines of code, it's repetitious, and its many if comparisons could be a performance problem if called in a tight loop.
The "math" solution is about as compact as a solution could be, and consequently it's quite elegant too. On the other hand, that makes it harder to read, it's not as flexible for potential future changes, and its multiple string allocations could be its own sort of performance problem.
The third one is a blend that we can call the "array" solution:
private static readonly string[] Rounds = {
"⚪⚪⚪⚪⚪⚪⚪⚪⚪⚪",
"🔵⚪⚪⚪⚪⚪⚪⚪⚪⚪",
"🔵🔵⚪⚪⚪⚪⚪⚪⚪⚪",
"🔵🔵🔵⚪⚪⚪⚪⚪⚪⚪",
"🔵🔵🔵🔵⚪⚪⚪⚪⚪⚪",
"🔵🔵🔵🔵🔵⚪⚪⚪⚪⚪",
"🔵🔵🔵🔵🔵🔵⚪⚪⚪⚪",
"🔵🔵🔵🔵🔵🔵🔵⚪⚪⚪",
"🔵🔵🔵🔵🔵🔵🔵🔵⚪⚪",
"🔵🔵🔵🔵🔵🔵🔵🔵🔵⚪",
"🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵",
};
private static string GetPercentageRounds(double percentage)
{
return Rounds[(int)Math.Ceiling(percentage * (Rounds.Length - 1))];
}
This one has the same at-a-glance readability of the original solution, it's even more flexible since it works with an arbitrary number of loading stages, and it makes no dynamic string allocations. On the other hand, it requires a separate static readonly string[] that's external to the method (hampering readability), and its main advantage over the simpler original or "math" solutions is just performance, something which is likely a premature optimization for a method like this (particularly since we haven't actually measured to confirm that it is faster).
So, Which Solution Is Best?
None of them and all of them, I would argue. A developer might have their own preference, but none of the solutions are truly so bad that they ought to be condemned in a code review.
The debate over this snippet of code reminds me how hard software development can be: here's a piece of code which is trivial enough that debating it is almost certainly bike-shedding, but even so, that debate demonstrates just how many decisions a developer needs to make even during the easiest parts of their day. Best to accept that, for trivial code like GetPercentageRounds(), maybe an embarrassingly simple and repetitious solution is just fine, and to save our energy for debating the solutions to the complex and mission-critical problems where the subtleties are more impactful.