4 ms·

In 'Suspicious comparison of real numbers': isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold))); is called out as bad code by the blog

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...

bloomer 8y ago
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.
scscsc 8y ago
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.
- geofft 8y ago
  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.
- melkiaur 8y ago
  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.
- benj111 8y ago
  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.
lifthrasiir 8y ago
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/ApplicationArchitecture.md#propertychanged-events https://github.com/Microsoft/calculator/blob/master/docs/App...
- amedvednikov 8y ago
  Wow, I can imagine how much fun it is to debug such properties.

bloomer 8y ago

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.

scscsc 8y ago

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.

geofft 8y ago
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.
melkiaur 8y ago
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.
benj111 8y ago
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.

lifthrasiir 8y ago

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/ApplicationArchitecture.md#propertychanged-events https://github.com/Microsoft/calculator/blob/master/docs/App...

amedvednikov 8y ago
Wow, I can imagine how much fun it is to debug such properties.