rspec/rules/S2760/comments-and-links.adoc

=== 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]