8 ms·

Example: https://imgur.com/a/9cIDQtk https://imgur.com/a/9cIDQtk

• jpnc 4y ago
  Is this literate programming?
  • sam_lowry_ 4y ago
    It's been extensively discussed on twitter, and the general conclusion seems to be that yes, this particular snippet is good code.
    • Freak_NL 4y ago
      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.
      • kadoban 4y ago
        In any language I can think of, 0 and 0.0 are the same, once you're comparing them against a double.
        • DoingIsLearning 4y ago
          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.
    • LudwigNagasena 4y ago
      Discussed by whom? By people who deem every non-built-in data structure as "too clever" for maintenance?
    • geraldwhen 4y ago
      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.
      • icoder 4y ago
        If that's the intended behaviour (having those boundaries and those results), how can you ever confirm that behaviour without checking them all?
        • doodlesdev 4y ago
          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.
• yread 4y ago
  I like it. Easy to understand, fast, no allocations.
  • kwhitefoot 4y ago
    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.
    • Luc 4y ago
      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.
      • kwhitefoot 4y ago
        How long do I need to wait?
        • dayvan 4y ago
          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.
        • Luc 4y ago
          It's just too obvious. The metrics you're optimizing for don't matter to any of the stakeholders.
• pelorat 4y ago
  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.
  • lucumo 4y ago
    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.
    • googlryas 4y ago
      You don't know what tooling anyone editing your code is using though.
      • Thiez 4y ago
        If you enforce correct formatting before commits or in your CI builds that is no longer a problem.
        • kwhitefoot 4y ago
          That's a big if though.
• rsynnott 4y ago
  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.
  • doodlesdev 4y ago
    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 https://en.m.wikipedia.org/wiki/Percentage
    • rsynnott 4y ago
      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.
      • doodlesdev 4y ago
        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.
• c7DJTLrn 4y ago
  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.
  • doodlesdev 4y ago
    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.
    • jeroenhd 4y ago
      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.
  • arp242 4y ago
    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.