Hacker News Reader

Comment by kf | Hacker News Reader

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.

So many big egos in software.

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.

I mean, if this is the worst code people can come up with then it's better than most codebases I've had to deal with at $dayjob.

I like it. Easy to understand, fast, no allocations.

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.

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.

How long do I need to wait?

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.

It's just too obvious. The metrics you're optimizing for don't matter to any of the stakeholders.

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.

Find-and-replace exists.

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.

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.

[0]: https://en.m.wikipedia.org/wiki/Percentage

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.

Nah, I was half-joking. This one is a really common bit of confusing naming in English, as well.

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.

You don't know what tooling anyone editing your code is using though.

If you enforce correct formatting before commits or in your CI builds that is no longer a problem.

That's a big if though.

Is this literate programming?

It's been extensively discussed on twitter, and the general conclusion seems to be that yes, this particular snippet is good code.

The only thing that jumps out at me is this:

    if (percentage == 0)
        return […];
    if (percentage > 0.0 && […]

Can a double have a value that is larger than 0 but smaller or equal to 0.0? I would have expected '> 0' instead.

In any language I can think of, 0 and 0.0 are the same, once you're comparing them against a double.

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.

Discussed by whom? By people who deem every non-built-in data structure as "too clever" for maintenance?

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.

Pull request denied.

If that's the intended behaviour (having those boundaries and those results), how can you ever confirm that behaviour without checking them all?

You could have one check such as

  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.

Reply on news.ycombinator.com