163 lines
4.5 KiB
Plaintext
163 lines
4.5 KiB
Plaintext
=== is duplicated by: S2751
|
|
|
|
=== relates to: S1862
|
|
|
|
=== on 24 Apr 2015, 08:15:30 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] This rule gives false positives on the following pattern:
|
|
|
|
----
|
|
if (a == null) {
|
|
a = tryToGetA();
|
|
}
|
|
if (a == null) {
|
|
throw new Exception();
|
|
}
|
|
...
|
|
----
|
|
|
|
Should we add an exception to this rule to check if the condition of the ``++if++`` is comparing an expression, and inside the ``++if++`` some value is assigned to the same expression? Or should this pattern be avoided?
|
|
|
|
=== on 27 Apr 2015, 07:57:37 Ann Campbell wrote:
|
|
\[~tamas.vajk] see what you think
|
|
|
|
=== on 27 Apr 2015, 08:28:29 Evgeny Mandrikov wrote:
|
|
\[~ann.campbell.2] as I already told to [~tamas.vajk] : I don't agree with such exception, IMO provided example doesn't show really readable code - why one wouldn't combine first two if-statements ?
|
|
|
|
=== on 27 Apr 2015, 15:40:32 Ann Campbell wrote:
|
|
Exception reverted
|
|
|
|
=== on 3 Jun 2015, 07:34:05 Evgeny Mandrikov wrote:
|
|
\[~ann.campbell.2] maybe we should add a little explanation why this is irrelevant for JavaScript? to prevent confusion during further reviews?
|
|
|
|
=== on 3 Jun 2015, 14:29:46 Ann Campbell wrote:
|
|
Irrelevant for JavaScript because this is a very common pattern, which raised hundreds of FP's
|
|
|
|
----
|
|
if ( ! isItOkay(a)) {
|
|
doSomethingTo(a);
|
|
}
|
|
if ( ! isItOkay(a)) {
|
|
// try again? raise error? ...
|
|
}
|
|
----
|
|
cc [~elena.vilchik]
|
|
|
|
=== on 3 Jun 2015, 14:35:48 Evgeny Mandrikov wrote:
|
|
\[~elena.vilchik], [~ann.campbell.2], same question as for example from C# - why one wouldn't combine those two if-statements into single one?
|
|
|
|
----
|
|
if ( ! isItOkay(a)) {
|
|
doSomethingTo(a);
|
|
if ( ! isItOkay(a)) {
|
|
// try again? raise error? ...
|
|
}
|
|
}
|
|
----
|
|
|
|
=== on 4 Jun 2015, 13:09:44 Tamas Vajk wrote:
|
|
\[~evgeny.mandrikov] we had the same in C# as we discussed it earlier. Since then, I added an exception for the following:
|
|
|
|
----
|
|
if (condition)
|
|
{
|
|
statement
|
|
}
|
|
if (condition)
|
|
{
|
|
//...
|
|
}
|
|
----
|
|
|
|
where ``++statement++`` is in any of the following three formats:
|
|
|
|
----
|
|
x = anything; //any kind of assignment (-=, +=, ...)
|
|
x++; //any postfix unary
|
|
++x; //any prefix unary
|
|
----
|
|
and ``++x++`` can be any symbol in ``++condition++``.
|
|
|
|
|
|
With this simple exception, I could get rid of all the false positives that we had in the C# ruling.
|
|
|
|
|
|
|
|
=== on 4 Jun 2015, 14:07:55 Evgeny Mandrikov wrote:
|
|
\[~tamas.vajk] I see the issue here - implementation not aligned with RSPEC. /CC [~dinesh.bolkensteyn] [~ann.campbell.2] [~freddy.mallet]
|
|
|
|
=== on 4 Jun 2015, 15:31:13 Dinesh Bolkensteyn wrote:
|
|
As the RSPEC is assigned to you [~evgeny.mandrikov] , I understand that you take care to come up with a solution to this issue.
|
|
|
|
=== on 5 Jun 2015, 12:09:48 Evgeny Mandrikov wrote:
|
|
\[~dinesh.bolkensteyn] no, I don't, I don't know why Ann assigned it back to me.
|
|
|
|
=== on 5 Jun 2015, 12:50:22 Ann Campbell wrote:
|
|
Absentmindedness [~evgeny.mandrikov]. I thought it was _still_ assigned to me, not _reassigned_ to me.
|
|
|
|
=== on 5 Jun 2015, 23:24:19 Evgeny Mandrikov wrote:
|
|
\[~tamas.vajk] if I correctly understand your exclusion, then this case will be excluded:
|
|
|
|
{noformat}
|
|
|
|
if (c > 5)
|
|
|
|
{cpp};
|
|
|
|
if (c > 5) // copy-paste error
|
|
|
|
c--;
|
|
|
|
{noformat}
|
|
|
|
I don't see an excuse for successive if-statements with identical conditions - they are error-prone and IMO make code less unreadable.
|
|
|
|
|
|
|
|
=== on 8 Jun 2015, 07:59:28 Tamas Vajk wrote:
|
|
\[~evgeny.mandrikov] Yes, you see it correctly, this won't report an issue in the current implementation.
|
|
|
|
=== on 8 Jun 2015, 08:04:06 Dinesh Bolkensteyn wrote:
|
|
my 2 cents: This is a maintainability rule, not a bug detection one - there should be another rule that targets the discovery of copy&paste errors
|
|
|
|
|
|
----
|
|
if (foo == null)
|
|
{
|
|
foo = GetFoo1();
|
|
}
|
|
|
|
if (foo == null)
|
|
{
|
|
foo = GetFoo2();
|
|
}
|
|
----
|
|
|
|
this could be refactored into:
|
|
|
|
|
|
----
|
|
if (foo == null)
|
|
{
|
|
foo = GetFoo1();
|
|
if (foo == null)
|
|
{
|
|
foo = GetFoo2();
|
|
}
|
|
}
|
|
----
|
|
|
|
but there clearly is no bug here...
|
|
|
|
=== on 8 Jun 2015, 08:43:28 Dinesh Bolkensteyn wrote:
|
|
Let's discuss IRL
|
|
|
|
=== on 24 Jun 2015, 19:07:24 Ann Campbell wrote:
|
|
\[~tamas.vajk], [~evgeny.mandrikov], [~dinesh.bolkensteyn] where do we stand on this rule? FYI, I've just upgraded severity to Critical since I'm pretty sure we want to keep the 'bug' tag.
|
|
|
|
=== on 25 Jun 2015, 13:01:35 Tamas Vajk wrote:
|
|
\[~ann.campbell.2], I've added an exception to this rule in the C# substask.
|
|
|
|
=== on 3 Sep 2015, 13:51:39 Ann Campbell wrote:
|
|
Downgraded from Critical/bug/Reliability to Major/suspicious/Maintainability and exception added after discussion with [~freddy.mallet], [~tamas.vajk], [~ann.campbell.2]
|
|
|