Avoid false positives in MemberNameEqualsClassName #4329
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4329 +/- ##
=========================================
Coverage 84.22% 84.22%
- Complexity 3258 3259 +1
=========================================
Files 474 474
Lines 10322 10323 +1
Branches 1825 1826 +1
=========================================
+ Hits 8694 8695 +1
Misses 666 666
Partials 962 962
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| private fun isFactoryMethod(function: KtNamedFunction, klass: KtClass): Boolean { | ||
| private fun isFactoryMethod(function: KtNamedFunction, klass: KtClass): Boolean? { |
There was a problem hiding this comment.
Boolean -> Boolean? sounds like a code smell to me.
Are we able to invert the logic here and make return true if is a Factory and false if we either don't know or it's not?
There was a problem hiding this comment.
Why do you think this is a code smell? The point of null to have a value for "we don't know" and it seems like a good usage of null.
Are we able to invert the logic here and make return
trueif is a Factory andfalseif we either don't know or it's not?
It should be the other way around: I want to the same if we know that it is a factory or if we don't know. So it should be: false if it is not a factory and true if we don't know or if it is a factory. And return true if "we don't know" seems odd. For that reason I introduced the null. I can change it for true if you think it is better. This is a private function so the correction of what it returns is not that important.
There was a problem hiding this comment.
Why do you think this is a code smell?
I think it's maybe more a matter of taste, but I'm in general not a big fan of Boolean?. It forces you to use == false/true in conditions and is not immediately obvious for the reader why the logic is ternary rather than binary.
It should be the other way around
Yup you're right. My point was anyway to restrict to be a Boolean and treat the absence of value as one of the two cases.
There was a problem hiding this comment.
I think it's maybe more a matter of taste, but I'm in general not a big fan of
Boolean?. It forces you to use== false/truein conditions and is not immediately obvious for the reader why the logic is ternary rather than binary.
Agree. I changed the code for a Boolean and I added a comment in the code to make clear that we don't know but we are assuming that it's a factory to avoid false-positives.
If we can't infer if it's a factory or not assume that it's ok. False-negatives are better than false-positives.
This rule shoul be moved to type-solving only but that's a huge change that probably can be done for 2.0
fixes #4070