It's pretty pathetic how many people feel the need to dunk on this bit of code just because it's not how they would write it. There's nothing really wrong with it. I'm sure the author was aware of alternative, perhaps more concise solutions using a string builder but they chose to be clear instead.
I'm pretty sure they weren't because of the redundant conditionals which simply defy logic. If there was only one check for every if statement, honestly I could give this a pass since it's at the very least simple, but by adding one extra redundant check for every statement you just created 9 new places where a bug could appear.
Furthermore, using Unicode characters to represent progress is the true smell here. There simply are better ways to do this.
In the grand scheme of things, does it matter? No. But this is Hacker News LOL, someone has to discuss it.
If I had to show a progress bar for less than a second in a screen the user will only open up once per 10 years (it's NFC code for scanning passports/ID cards), I wouldn't bother writing a reusable custom progress bar component either.
Sure, you can do it better, but why would you? There are other, more pressing issues in this code (that probably also don't warrant spending extra time on refactoring).
Those redundant checks are highlighted in every IDE I can think of. I can only assume they're there for readability.
It has almost twice as many comparisons as necessary. The term to the left of each AND is redundant because it has already been checked by the preceding IF. It also does not guard against negative arguments. Perhaps the environment in which it is used guarantees that negative arguments cannot occur.
If I were reviewing this code I would at least ask the developer to add an assertion or contract requiring that the argument be in the inclusive range [0..1]
The choice of variable name, percentage, is also misleading. At least I suspect it is because I would expect the comparisons involving percentages to be to numbers between 0 and 100.
If lack of allocations is a requirement then one could create a static array of strings and use
int(percent * 10)
as the index. This would eliminate all of the comparisons and also throw an index out of range (in any sane language) if the value was outside the allowed range.
If you have int(percent * 10) + 1 you can just generate that many blue circles (checking for the edge-case of zero, or even better using ceil instead of int), the rest white and return it - no need for manually crafting the array (since the performance is, I presume, not a critical thing here). If tomorrow you want stars instead of the circles you just edit 2 chars in one place, instead of typing manually all combinations.
The compile time allocated array is to avoid allocations at run time, if that is a requirement. In a language with proper macros such as Nim or Lisp this can be done at compile time using exactly your approach. That way it executes fast and is just as simple .
I've been looking into nim lately (just for fun with the Advent of Code problems) and it looks fantastic. I plan to allocate more time to it in future definitely.
Having a separate string for each level of progress also lets you do other kinds of customizations: you could have a rainbow progress bar, or put little bits of encouraging text to the right of the progress bar, like "Almost there!" at 90%.
Essentially, you're making one type of customization (i.e., changing the symbols) slightly easier, at the expense of making other types of customization harder.
You know it's only a matter of time before someone dissects each one of your objections. In fact you could do so yourself with a bit of a wider perspective.
I think they're all great suggestions (albeit for such a tiny, irrelevant piece of code). The only problem I can think of is that the given code rounds up, but your suggestion of `int(percent * 10)` rounds down.
I vaguely suspect that this is a product of the sort of environment where you have to fill out a form in triplicate to get the static analyser to let you concatenate strings (which, to be clear, may not be inappropriate for something like this).
I do object to the variable being called ‘percentage’ tho, as it clearly isn't one.
I have no idea where all of you got the idea that percentages go up to 100. It's in the name: PER centage, meaning x/100 [0].
For instance if you want 20% that could also be expressed as a fraction such as 20/100, which turns out is the same as 2/10 or 0.2.
I do think they should remove the redundant statements in the conditions and also have an assertion that guarantees percentage to be [0, 1].
> The term "percent" is derived from the Latin per centum, meaning "hundred" or "by the hundred". The sign for "percent" evolved by gradual contraction of the Italian term per cento, meaning "for a hundred". The "per" was often abbreviated as "p."—eventually disappeared entirely. The "cento" was contracted to two circles separated by a horizontal line, from which the modern "%" symbol is derived.
This might be a little more obvious for me since my first language is derived from Latin, but anyhow it still keeps the meaning in english.
20 percent means, literally, 20 per hundred; it's equivalent to 0.2 or 2/10 or 1/5 or whatever, of course, but if `percentage==0.2` then that fairly clearly, on the face of it, should mean "0.2 per hundred", ie 0.2% or 0.002.
It really shouldn't. 20% means _literally_ 20 / 100 so if you need to express that numerically (as you do in code since % is reserved for modulo) you write that as 0.2. That is still a percentage, just in numerical decimal form instead of in the form of a fraction, the value is exactly the same and it didn't stop being a percentage.
If I write 0.2 in a piece of paper and give it to someone and tell them that's a percentage it should be pretty obvious that means it's 20%. If you do the same but you write 0.2% then of course it's 0.2%.
If they really wanted to they could've written the comparison using the numbers as fractions in the comparisons such as percentage < 10/100 which would be perfectly reasonable, but again, that resolves to 0.1, so you might as well right it in decimal form already.
This is likely an effect of translation more than anything. While the Dutch are generally very competent English speakers and writers, their expertise tends to end the conversational level. Anything technical in its conception takes decades of intense every day use to intuit.
Source: native English speaker working in the Netherlands with a team of Dutch people. They are all really smart people, but they tend to err on the side of simple vocabulary when forced to think in English.
I think another cause is that english tends to have simpler sentence structures when compared to dutch in the first place, and dutch folks tend to over-correct towards simplicity when speaking/writing/thinking english.
E.g. this is a perfectly cromulent dutch sentence:
"Vorig jaar zijn we gestart met scholing rondom systeemdenken met als doel de lessen rond begrijpend lezen naar een hoger niveau te tillen en de leesresultaten van de kinderen te verbeteren."
Which when fairly directly translated to english ends up something like:
"Last year we have started with schooling around system thinking with as goal lifting the classes on reading comprehension to a new level and improving the reading results for the children."
which while valid english, isn't very idiomatic -- never mind hard to parse. A native would most likely split this into three or four sentences. E.g.:
"Last year we started with schooling around system thinking. The goal of his is to lift the classes on reading comprehension to a new level. Simultaneously this will improve the childrens' learning results."
I'm triggered by the lack of brackets after every if-expression. Sure it looks nicer this way but the default Visual Studio code style settings will complain if you don't do it, hence I'm used to it.
I've started to remove them from my own code. It's widely mentioned as The Right Way, but I feel the reasons why are obsolete. The stated reason is always that you could forget to add braces when adding a second statement.
That was useful in a time where a text editor was "smart" when it copied your indentation to a new line. But nowadays any tooling will warn you when indentation doesn't match the bracing. The odds of people making that mistake has gone so far down, that the risk is no longer worth the reduced readability.
Technically what is happening behind the scenes is that for most languages the compiler/interpreter will promote the integer to a double to avoid foot guns.
Nevertheless integer comparisons with any kind of floating point is not a wise choice.
The idiomatic way to compare a double would be to take into account whatever is the double precision epsilon for that language. Or just use the greater/less than like they have in the subsequent if statements in the original code snippet.
It’s not, though. To confirm the method works you need to check every single comparison operator and value to ensure the range is bounded correctly. It’s code that stops you in your tracks.
if(percentage < 0 or percentage > 1)
{
// Throw error here
}
Also the checks in the if statements in the linked code are redundant since they simply disregard the previous check, they could simply check if percentage < x instead of checking it's within a range sincs the previous check already proved percentage to be > x - 1/10.
To be fair though, this is the kind of code where "if it's stupid and it works, it's not stupid" applies perfectly. While I would make these changes if I had to approve a PR I wouldn't change this in a live codebase just for refactoring purposes, specially because there are better ways to show progress to a user than using Unicode characters, which I think is the real smell here.
So many big egos in software.