Hacker News Reader

Comment by sonofgod | Hacker News Reader

In 'Suspicious comparison of real numbers':

isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold)));

is called out as bad code by the blogger; this feels very much like a false positive, given that we're checking an inequality already; just effectively changing it from > to >=

A good example of not just blindly following the linter...

View on HN

Yet another example of "suspicious" test code which intention is very clear:

    TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing)
    {
      // *snip*
      vm.Value2Active = true;
      // Establish base condition
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
      vm.Value2Active = true;
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
    }

> [...] The analyzer has detected two identical code fragments executing immediately one after the other. It looks like this code was written using the copy-paste technique and the programmer forgot to modify the copies.

Here Value2Active is a C++/CX property rigged to a PropertyChanged event [1] with a macro:

    public ref class UnitConverterViewModel sealed: public Windows::UI::Xaml::Data::INotifyPropertyChanged
    {
        // other members omitted
        OBSERVABLE_OBJECT();
        OBSERVABLE_PROPERTY_RW(bool, Value2Active);
    };

Can you be sure that this actually does not have a side effect? I bet not. Indeed, the intention of the original test is clear: a property, once set ("Switch") to a particular value, should not update other states even when set ("Reselect") to that same value---as it will really trigger a side effect! This idempotency guarantee is an important interface contract worth testing, no matter what a linter and clueless blog author says.

[1] https://github.com/Microsoft/calculator/blob/master/docs/App...

Wow, I can imagine how much fun it is to debug such properties.

Yeah that whole section on suspicious comparison looks like false positives by someone who didn't even bother to look at the code or doesn't understand it. Right after there is also the flagging of strod(rounded string) = 0.0 which I'm pretty sure is correct. They are checking whether the rounded string is exactly zero. Automated tooling is really only useful if you actually know what you are doing and can properly triage the output. "Look I ran this tool and it spit out some things" isn't very helpful.

Still, the comparison ratio == threshold has very little chance to turn out true.

When you want to check whether A >= B (where A and B are numbers represented as floats), you would probably want to use the check A >= B - epsilon.

It only has little chance to turn out true if they're calculated in different ways and accumulate different error. If you're e.g. dividing two pairs of integers and comparing them, regular equality is likely to be fine.

A relevant question is what epsilon should be.

I disagree strongly. It looks obvious to me that they can be equal and that this is a special case. The comparison is done only after checking for A<B, and only if a boolean called "ActiveIfEqual" is true. I mean, I don't know how much more obvious they could have made it.

All edge cases have very little chance of being true, and I would hazard a guess that most of a developers time, and most code is spent on edge cases.

So I'm not sure you can argue against a piece of code on the basis of likelihood.

Reply on news.ycombinator.com