Code sniff for verbose conditionals

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Code sniff for verbose conditionals

Daimona
Currently, there's a patch under review [0] to enable a PHP sniff which
would detect so called "Pointless conditionals". These are conditionals
(if...else or ternary) where both branches only assign or return a boolean
value, and which could potentially one-lined using the if condition. T
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Daimona
(Pardon, I hit "send" while typing :/)

I was saying, you can find several examples of wrong and correct code at
[1].
Thiemo pointed out that this patch could encourage to write shorter and
less readable code (you can find his rationale in Gerrit comments), and I
partly agree with him. My proposal is to only trigger the sniff if the "if"
conditions are "short" enough. Which could mean either a single condition,
shorter than xxx characters etc.
We're looking for some feedback to know whether you would deem this feature
useful, and how to tweak it in order to avoid encouraging bad code.
Thanks to you all, and again - sorry for the half message.
Daimona

[0] -
https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/codesniffer/+/486813/
[1] -
https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/codesniffer/+/486813/16/MediaWiki/Tests/files/ControlStructures/pointless-conditional.php


Il giorno sab 9 feb 2019 alle ore 12:30 Daimona <[hidden email]> ha
scritto:

> Currently, there's a patch under review [0] to enable a PHP sniff which
> would detect so called "Pointless conditionals". These are conditionals
> (if...else or ternary) where both branches only assign or return a boolean
> value, and which could potentially one-lined using the if condition. T
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Stas Malyshev
Hi!

> I was saying, you can find several examples of wrong and correct code at
> [1].
> Thiemo pointed out that this patch could encourage to write shorter and
> less readable code (you can find his rationale in Gerrit comments), and I
> partly agree with him. My proposal is to only trigger the sniff if the "if"
> conditions are "short" enough. Which could mean either a single condition,
> shorter than xxx characters etc.
> We're looking for some feedback to know whether you would deem this feature
> useful, and how to tweak it in order to avoid encouraging bad code.
> Thanks to you all, and again - sorry for the half message.

This looks useful. I think PHPStorm has this check built in, but having
it in sniffs too wouldn't be a bad thing. I've seen such things happen
when refactoring code.

--
Stas Malyshev
[hidden email]

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Kosta Harlan
Hi Daimona,

Thanks for working on this. Have you run this against sniff against mw core or extensions? If so it would be useful to look at examples of what it’s catching in existing code.

Kosta

> On Feb 9, 2019, at 9:26 PM, Stas Malyshev <[hidden email]> wrote:
>
> Hi!
>
>> I was saying, you can find several examples of wrong and correct code at
>> [1].
>> Thiemo pointed out that this patch could encourage to write shorter and
>> less readable code (you can find his rationale in Gerrit comments), and I
>> partly agree with him. My proposal is to only trigger the sniff if the "if"
>> conditions are "short" enough. Which could mean either a single condition,
>> shorter than xxx characters etc.
>> We're looking for some feedback to know whether you would deem this feature
>> useful, and how to tweak it in order to avoid encouraging bad code.
>> Thanks to you all, and again - sorry for the half message.
>
> This looks useful. I think PHPStorm has this check built in, but having
> it in sniffs too wouldn't be a bad thing. I've seen such things happen
> when refactoring code.
>
> --
> Stas Malyshev
> [hidden email]
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Daimona
Hi,
All patches in the codesniffer repo have a sample run against mwcore set up
in CI. As can be seen in [0], the current version is triggered 13 times by
MW core. No idea about extensions, though.
Daimona

[0]:
https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/console

Il giorno lun 11 feb 2019 alle ore 17:49 Kosta Harlan <[hidden email]>
ha scritto:

> Hi Daimona,
>
> Thanks for working on this. Have you run this against sniff against mw
> core or extensions? If so it would be useful to look at examples of what
> it’s catching in existing code.
>
> Kosta
>
> > On Feb 9, 2019, at 9:26 PM, Stas Malyshev <[hidden email]>
> wrote:
> >
> > Hi!
> >
> >> I was saying, you can find several examples of wrong and correct code at
> >> [1].
> >> Thiemo pointed out that this patch could encourage to write shorter and
> >> less readable code (you can find his rationale in Gerrit comments), and
> I
> >> partly agree with him. My proposal is to only trigger the sniff if the
> "if"
> >> conditions are "short" enough. Which could mean either a single
> condition,
> >> shorter than xxx characters etc.
> >> We're looking for some feedback to know whether you would deem this
> feature
> >> useful, and how to tweak it in order to avoid encouraging bad code.
> >> Thanks to you all, and again - sorry for the half message.
> >
> > This looks useful. I think PHPStorm has this check built in, but having
> > it in sniffs too wouldn't be a bad thing. I've seen such things happen
> > when refactoring code.
> >
> > --
> > Stas Malyshev
> > [hidden email]
> >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy (he/him)
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Bartosz Dziewoński
On 2019-02-11 18:42, Daimona wrote:
> Hi,
> All patches in the codesniffer repo have a sample run against mwcore set up
> in CI. As can be seen in [0], the current version is triggered 13 times by
> MW core. No idea about extensions, though.
> Daimona
>
> [0]:
> https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/console

Thanks for that link. I looked at them and submitted a change (please do
not merge it) to demonstrate what changes this would require:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/489759

In my opinion most of these changes are clear improvement or harmless,
except for the pattern in LBFactorySimple.php/LoadBalancer.php, which is
a little tricky and probably clearer in the original version.

--
Bartosz Dziewoński

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Daimona
Thanks for the change! I just realized that my patch has another problem:
when checking assignments, it should also check that the receiver is the
same in both branches, and this would avoid the case in LoadBalancer.
I also find excessive the case in GlobalFunctions, and that would be solved
by implementing a check on the complexity of the 'if' condition, as
proposed in gerrit comments. Ideas on how to perform such check are welcome.

Il giorno lun 11 feb 2019 alle ore 19:42 Bartosz Dziewoński <
[hidden email]> ha scritto:

> On 2019-02-11 18:42, Daimona wrote:
> > Hi,
> > All patches in the codesniffer repo have a sample run against mwcore set
> up
> > in CI. As can be seen in [0], the current version is triggered 13 times
> by
> > MW core. No idea about extensions, though.
> > Daimona
> >
> > [0]:
> >
> https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/console
>
> Thanks for that link. I looked at them and submitted a change (please do
> not merge it) to demonstrate what changes this would require:
> https://gerrit.wikimedia.org/r/c/mediawiki/core/+/489759
>
> In my opinion most of these changes are clear improvement or harmless,
> except for the pattern in LBFactorySimple.php/LoadBalancer.php, which is
> a little tricky and probably clearer in the original version.
>
> --
> Bartosz Dziewoński
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
https://meta.wikimedia.org/wiki/User:Daimona_Eaytoy
"Daimona" is not my real name -- he/him
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Trey Jones
In reply to this post by Daimona
I decided to look at some examples, and I found one that gives me pause.[0]
if ( $i == 0 ) {
$this->servers[$i]['master'] = true;
} else {
$this->servers[$i]['replica'] = true;
}

I don't know what's specifically going on here, but it's possible that only
$this->servers[$i]['master'] or $this->servers[$i]['replica'] is ever set
(rather than both being previously set to false, for example), so something
like this could *possibly* break later code (that would be some brittle
code, but worse things have been done):
$this->servers[$i]['master'] = ( $i == 0 );
$this->servers[$i]['replica'] = !$this->servers[$i]['master'];

I'm not sure how else to refactor this to avoid the pointless conditional
failure.

That said, thanks for the work to continue to improve our code base!
—Trey

[0]
https://gerrit.wikimedia.org/g/mediawiki/core/+/6968592a9acd683cb7fee4b0f7d6056ae5987c89/includes/libs/rdbms/lbfactory/LBFactorySimple.php#62

Trey Jones
Sr. Software Engineer, Search Platform
Wikimedia Foundation


On Mon, Feb 11, 2019 at 12:43 PM Daimona <[hidden email]> wrote:

> Hi,
> All patches in the codesniffer repo have a sample run against mwcore set up
> in CI. As can be seen in [0], the current version is triggered 13 times by
> MW core. No idea about extensions, though.
> Daimona
>
> [0]:
>
> https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/console
>
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Trey Jones
I see that my case has already been found by Bartosz, so disregard my
message. Sorry!

On Mon, Feb 11, 2019 at 3:36 PM Trey Jones <[hidden email]> wrote:

> I decided to look at some examples, and I found one that gives me pause.[0]
> if ( $i == 0 ) {
> $this->servers[$i]['master'] = true;
> } else {
> $this->servers[$i]['replica'] = true;
> }
>
> I don't know what's specifically going on here, but it's possible that
> only $this->servers[$i]['master'] or $this->servers[$i]['replica'] is
> ever set (rather than both being previously set to false, for example),
> so something like this could *possibly* break later code (that would be
> some brittle code, but worse things have been done):
> $this->servers[$i]['master'] = ( $i == 0 );
> $this->servers[$i]['replica'] = !$this->servers[$i]['master'];
>
> I'm not sure how else to refactor this to avoid the pointless conditional
> failure.
>
> That said, thanks for the work to continue to improve our code base!
> —Trey
>
> [0]
> https://gerrit.wikimedia.org/g/mediawiki/core/+/6968592a9acd683cb7fee4b0f7d6056ae5987c89/includes/libs/rdbms/lbfactory/LBFactorySimple.php#62
>
> Trey Jones
> Sr. Software Engineer, Search Platform
> Wikimedia Foundation
>
>
> On Mon, Feb 11, 2019 at 12:43 PM Daimona <[hidden email]> wrote:
>
>> Hi,
>> All patches in the codesniffer repo have a sample run against mwcore set
>> up
>> in CI. As can be seen in [0], the current version is triggered 13 times by
>> MW core. No idea about extensions, though.
>> Daimona
>>
>> [0]:
>>
>> https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/966/console
>>
>>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Code sniff for verbose conditionals

Thiemo Kreuz
In reply to this post by Daimona
Sorry it took me so long to respond here.

In https://gerrit.wikimedia.org/r/486813 Gergő wrote:

> […] it adds some fairly complicated code for detecting a pattern that's mostly harmless and can be dealt with during normal code review, so the value is less than the maintenance cost.

Thanks! I think this sums it up really well.

> we should aim for a threshold which would only reject code that is clearly wrong […]

See, that's the problem: I don't think a single of the examples I have
seen so far is "clearly wrong". I don't think there is anything
"wrong" with explicitly stating all possible return values. Yea, one
*might* consider some of the examples a little tooo expressive. Some
of them *can* be shortened. But when this is the case and when not is
more an opinion than anything, and needs to be talked about in a code
review.

> […] doesn't mean that we should avoid dealing with it without even trying.

I don't think this question was answered yet: What are we even trying
to solve here? How big of an issue is this? How often do we come
across such code during our code reviews and feel "I wish a sniff
would have found this before I did"? I do a ton of code reviews. Yes,
there is code like this. But from my personal experience this is so
rare and so much a matter of personal preference and opinion that it's
not worth dealing with the maintenance costs a fully-automated sniff
comes with.

> Could you provide specific examples where my proposed approach produces an undesirable outcome? That is, an example of code you believe should be acceptable but would be marked as fixable?

I'm sorry, but I believe it should not work this way around. I would
like to point to the examples in the test file. Sure, some of them
*can* be rewritten in a shorter way. But none of them *must* be
rewritten.

CodeSniffer rules are not to make suggestions (actually they are, but
that's not how we use them). The moment we make a sniff report certain
patterns, somebody will come and try to "fix" it, no matter how much
sense it makes. Not long ago a sniff complained about uncommented
@param tags, and people started adding cruft like `@param User $user
The user object`. I would like to avoid running into the same
situation again when we have much more pressing problems, e.g. bad
test coverage, or God objects with hundreds of static methods and
several thousand lines. *These* should make a sniff fail that forbids
making code too complex.

Kind regards
Thiemo

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l