Also available in Italian

On Twitter recently someone posted this picture:

Ok, that’s C#, but we can (and will, in a few lines) rewrite it in C++.

In the twitter responses, many people made fun of the code, or complained that the function is full of magic numbers, some others that it’s too long (implying that the cost of maintenance is proportional to the number of lines). And I was tempted to do the same, but something stopped me.

I realized nobody asked “What the problem was?”.

Let’s assume, for a moment, that the problem was exactly that – filling in precisely 10 balls, a number of balls proportional to the percentage. Is this a terrible solution?

So I decided, this is going to be today’s exercise – “wasting” an entire evening on a progress bar made of 10 balls.

The code

This is a C++ blog, so the first step will be rewriting the code in C++. I made it just a bit better, removing half of the constants… but I hope you’ll agree that, in spirit, it’s not too far off from the original.

const char * ProgressBugFixedSimplified(double percent)
{
    if (percent == 0.0) return "..........";
    if (percent <= 0.1) return "o.........";
    if (percent <= 0.2) return "oo........";
    if (percent <= 0.3) return "ooo.......";
    if (percent <= 0.4) return "oooo......";
    if (percent <= 0.5) return "ooooo.....";
    if (percent <= 0.6) return "oooooo....";
    if (percent <= 0.7) return "ooooooo...";
    if (percent <= 0.8) return "oooooooo..";
    if (percent <= 0.9) return "ooooooooo.";
    return "oooooooooo";
}

Does it solve the problem? Yes – almost.

The bugfix

My only contribution to the thread ended up being just this: fixing what I consider to be a bug.

For negative numbers, it returns a full bar, while in my opinion the bar should be empty. In some cases, indeed, numerical approximations can produce a slightly negative values when the numbers go around 0, and thus we could see a full bar when just at the beginning of our progress. It’s an easy fix: we just need to change the first test to be <=, and we’re done.

const char * ProgressBugFixedSimplified(double percent)
{
    if (percent <= 0.0) return "..........";
    if (percent <= 0.1) return "o.........";
    if (percent <= 0.2) return "oo........";
    if (percent <= 0.3) return "ooo.......";
    if (percent <= 0.4) return "oooo......";
    if (percent <= 0.5) return "ooooo.....";
    if (percent <= 0.6) return "oooooo....";
    if (percent <= 0.7) return "ooooooo...";
    if (percent <= 0.8) return "oooooooo..";
    if (percent <= 0.9) return "ooooooooo.";
    return "oooooooooo";
}

For the sake of this analysis, I took a few metrics: the code and the data size, number of calls and the presence of exceptions. In all cases, I used GCC 12.2, and the settings -O1 -std=c++20.

So, for this code, the results are:

Code size: 206 bytes
Data size: 121 bytes
Calls: none
Exceptions: no

Is there a shorter/better/smarter way to solve the problem? Hell yeah, we’re engineers, we can engineer the hell out of this function. Let’s start.

The loop

Ok, we need to collect enough balls to create our string, and fill the rest with dots. For simplicity, we’ll use std::string, just because we can.

std::string ProgressLoop(double percent)
{
    const int elements = 10;
    double threshold = percent * elements;
    std::string result = "";
    for (int i = 0; i < elements; ++i)
    {
        result += (i < threshold) ? "o" : ".";
    }
    return result;
}

Better?

I can change the number of balls, sure… but remember, that wasn’t in the requirements.

The code begins to be less readable, but we can still understand what’s happening.

Code size: 425 bytes (without external code)
Data size: 0 bytes
Calls: 8 – throw (3x), delete (2x), new (1x), unwind (1x), memcpy (1x)
Exception: yes

What a fail! the footprint is bigger, it allocates, deallocates, and copy memory, and it can even throw exceptions in three different points.

To be completely honest, we won’t probably incur in any of those calls, because this one will be well within the limits of SSO – Small String Optimization (see the nice blog post from Joel Laity libc++’s implementation of std::string for details). But still, the code is there.

Passing memory around?

We could avoid std::string and return an array of characters? No, that’s verboten.

Returning a pointer to char? Someone would need to allocate that, or receive the pointer via parameter, the interface would be more complex and error prone.

So, to cut it short… let’s see who…

…the winner is…

Here it is, my best non-allocating, string-like-returning, non-throwing, small-footprint version!

std::string_view Progress(double percent)
{
    constexpr char bar[] = "oooooooooo........";
    constexpr int elements = std::size(bar)/2;
    double threshold = percent * elements;
    return { &bar[int(elements - threshold)], &bar[int(2 * elements - threshold)] };
}

Nice, isn’t it?

The code is terribly small, the data is smaller than before, it just do some “simple” calculations, and solves the problem elegantly.

Except it’s wrong, as it goes out-of-bound every time you give a negative value, or a number above 1, crashing miserably.

But we can fix it, right? Sure…

std::string_view Progress(double percent)
{
    constexpr char bar[] = "oooooooooo.........";
    constexpr int elements = 10;
    double threshold = std::clamp<double>(percent * elements, 0, elements);
    return { &bar[int(elements - threshold)], &bar[int(2 * elements - threshold)] };
}

You love it already. I can feel it.

It’s just 4 lines, and one two are just declarations of constants, how hard it can be to maintain, right?

Right?

Wrong.

The code has a bug, and you didn’t even noticed it. The correct code is this:

std::string_view Progress(double percent)
{
    constexpr char bar[] = "oooooooooo..........";
    constexpr int elements = 10;
    double threshold = std::clamp<double>(percent * elements, 0, elements);
    return { &bar[int(elements - threshold)], &bar[int(2 * elements - threshold)] };
}

You can spot the difference, right? Well, the bug was not on one of the two uber-complex lines at the bottom, it was in the very first line. A missing dot. That would have caused a nice out-of-buffer read.

So, not even the “easy” part of this code is that easy to read or maintain, after all. And who can read the two bottom lines? Why the template parameter on clamp? Why the double doesn’t narrows to int in the array accessors? (no answer to these questions today, this is a simple blog post)

But let’s give the metrics anyway:

Code: 87
Data: 21
Calls: no
Exceptions: no

Good! Much better! We win!

Is it a victory?

The experience

I wanted to call this section “the verdict”, but I don’t feel like a judge today – I actually feel a bit stupid* for having initially laughed of the code, without realizing it was a simple and relatively elegant way to reach the goal.

But this made me think, and ask myself questions:

Can the original code be maintained by a junior developer (say, a new college graduate)? Yes.
What about my final code? Probably not. So we would need to pay a senior dev to maintain a progress bar.

Can the original code be written easily without a bug? Yes. And if bug happens, they’re easy to spot.
What about my final code? Not by me. To be clear, both bug on this post were intentional, but I did introduce other bugs while writing the intermediate code that didn’t appear in this code – if you’re lucky, you can still watch the entire 1h rant on twitch, and see my buggy code.

In the end, what is the cost of maintaining the original code? Probably small, it’s intuitive and easy to read.
What about my final code? In my opinion, terribly high. If I had to read that code 1 week from now, I’ll probably have to spend a minute or two to check if the call to clamp and the creation of string_view are correct.

And finally, the most important question of all was: would I, an engineer with 25+ years experience in C++, enjoy maintaining a progress bar function, just because nobody else in my team could efficiently do that?

No.

I might not change my coding style tomorrow, but this exercise definitely changed the way I’ll see the “maintenance cost”, and I’ll surely think about these aspects more often from now on.

* I’m always happy when I feel stupid, because it means I’ve learnt something.

Update 2023-01-23: The version I wrote was still buggy. Two of you (Vinzenz in the comments, and @emilianobr on Twitter) reported that the final code was buggy, and indeed I incorrectly assumed static was implied there (I seem to recall it worked on MSVC during my tests, but then failed on Clang and GCC when tested on godbolt). I’m not sure why Clang and GCC wouldn’t use a global constant for that (I need to re-read the standard about that, but nonetheless I’m pretty sure it’s my bug!). I feel this makes the entire content of this post even more valid.
Anyway, here’s the fixed code:

std::string_view Progress(double percent)
{
    static constexpr char bar[] = "oooooooooo..........";
    constexpr int elements = 10;
    double threshold = std::clamp<double>(percent * elements, 0, elements);
    return { &bar[int(elements - threshold)], &bar[int(2 * elements - threshold)] };
}

At the same time I fixed another couple of small mistakes

Update 2023-01-23 10:00 UTC: Fixed typos, and bugs around.

  • Missing fix in “fixed” version (reported by Fabian Sandin in the comments)
  • Typo in text (reported via a Tweet that has been then deleted).

15 Comments

  1. Tom

    Code review:
    – I think your complex code behaves different for small input values: the original code outputs one ball for input values like 0.0001. By casting the value from double to int the new code ends up with zero balls.
    – And your code would be more simple when using a constant of “……….oooooooooo” instead of “oooooooooo……….”.
    But your are right: too much brain energy for a simple problem.

    Reply
    • Marco Foco

      I didn’t wanted to change the original code semantics (they have 11 states, and 10 balls, with 0 = 0 balls, 1 = 10 balls, and no other cases for 0 and 10 balls). If I rounded, I would have had either different intervals, or less balls.
      Honestly, I find that the semantics they’ve given to this bar is sound:
      – 0 balls: no progress yet (process not started)
      – 1 ball: some progress made (process is started)

      – 9 balls: almost there, still some progress to do (process isn’t finished yet)
      – 10 balls: done (process finished)

      Rounding would make “very little progress” equal to “no progress”, and “almost there” equal to “done”.

      For the second of your points, I’m not sure how having `.`s before `o`s would have made the code simpler…

      Reply
      • Marco Foco

        Oh, I see now. I did change the semantics actually. Again, this is another good reason why that code should stay the way it was :)

        Reply
  2. Fabian Sandin

    You didn’t change the first test to ‘<=' in your second c++ example.

    Reply
    • Marco Foco

      Thank you, fixed :)

      Reply
  3. Vinzenz

    Hello. I tried to run this with godbolt.org and the output is memory garbage like ooo��e�2. Used c++20 and clang trunk.

    Reply
    • Vinzenz

      constexpr char bar[] = “oooooooooo……….”; is out of scope after levaing the function.

      Reply
      • Marco Foco

        You’re SOOO right. I was incorrectly assuming that in this context that would be staticly-initialized (I was fooled by MSVC working – It fails on Clang *and* GCC, indeed).
        Fixed.

        Reply
  4. Coudray

    A python solution :

    def print_bar(percentage):
    for n,num in enumerate(range(0,100,10)):
    print(‘o’, end=”) if percentage > num else print(‘-‘,end=”)

    Reply

  5. ‘Never try to be more royal than the king.’
    I would never spend so much time trying to improve functional and maintainable code.
    As an exercise for this article it is perfectly valid.
    Kudos!

    Reply
  6. Federico Vaccaro

    As a junior C++ dev, at the beginning I wanted to refactor the shit out of everything (and, afterward, getting crazy to get everything fitting together) this is definitely worth of reading :)

    Reply
  7. Rud Merriam

    I also thought the criticism of the original was misplaced. It is clear and understandable. I still worked for improvement, much as you did here. My work is in a Medium article:https://medium.com/one-stop-c-plus-plus/visiting-the-dutch-code-in-c-d6b342ab46be

    I wasn’t concerned about optimization since I was just playing around. My final code is:

    std::string GetPercentageRounds_Cpp_2(double percentage) {

    static std::string bubbles {std::string(10, ‘*’) + std::string(10, ‘o’)}; // duh! a single string of * & o

    percentage = std::ranges::clamp(percentage, 0.0, 1.0);
    int count = int(std::ceil(percentage * 10));
    return bubbles.substr(10 – count, 10);
    }

    As the comment says, I should have used a single string.

    Reply

  8. Nice post, and I like the message of not overengineering simple things.

    It’s a bit sad that I know *a lot* of people who would (mis-)read your post and wave it as a flag in their holy war against templates, , , and every other abstraction.

    I think you’ve taken a very toy example, for which there was truly no reason to optimize and, to be honest, not even any reason to question the code in the first place. This is a pure function. In whichever way it is written, I expect whoever wrote it to have guarded it with cross-platform exhaustive tests so that everybody else has never to look into it again. If the original Jon Doe had written it your way, they’d have found the static was missing by running tests, they’d have fixed it, and goodbye.

    Reply
    • Enrico Maria De Angelis

      s/templates, , ,/templates, ranges, corouotines,/

      Reply

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.