EarlyExitNotUsed sniff promotes inconsistent code organization

[morozov]

Consider an example:

<?php

function main($a, $b)
{
    if ($a !== null) {
        echo $a;
    }

    if ($b !== null) {
        echo $b;
    }
}

$ phpcs test.php 
FILE: test.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 9 | ERROR | [x] Use early exit to reduce code nesting.
   |       |     (SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed)
------------------------------------------------------------------------------------------------

The two if-statements are identical and have the same semantics. Using an early exit in the latter will make them look different and therefore will increas the cognitive load on the reader.

Besides that, even with one if-statement in the block, the sniff sometimes produces a sub-optimal suggestion:

<?php

function main($a, $b)
{
    if ($a !== null) {
        echo $a;
    }
}

$ phpcs test.php 
FILE: test.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 5 | ERROR | [x] Use early exit to reduce code nesting.
   |       |     (SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed)
------------------------------------------------------------------------------------------------

In this case, fixing this violation will not reduce code nesting and will add one more line of code.

I like the sniff itself to not disable it entirely, but it'd be nice if it was optimized for handling cases like the above.

[morozov]

@Majkl578 what do you think?

[Majkl578]

I noticed this earlier too.
In the first case it imho makes sense to keep both ifs as is, for consistency/readability.
In the second case I do prefer early exit though.

But this is really personal thing, at work, we had multiple discussions about this sniff and some colleagues didn't fully accept even the more obvious cases...

[morozov]

In the second case I do prefer early exit though.

I'd make an exception here and only report the violation if the block is larger than one line. Otherwise, the return statement which is proposed to be used instead has the same nesting level and size.

[kukulich]

But this is really personal thing

Agreed. That's why is a little difficult to satisfy everyone's needs. Currently I prefer not to do any exceptions in the sniff. You can always use phpcs:disable comment for a specific lines of code.

[dereuromark]

The "fixing" part should only be added where it won't break code.
Here it clearly would.

As such this must be just a warning (or at least a non fixable error) instead of the code flow continues as in this case.

[kukulich]

@dereuromark

The "fixing" part should only be added where it won't break code. Here it clearly would.

How would the sniff break the code?

[dereuromark]

Ah, I see the [x] is only there for the 2nd if. So it seems fine then. Nothing that can be done here then.

[Stadly]

only report the violation if the block is larger than one line.

What about making the sniff customizable, so that the smallest block size that the violation should be reported for can be set as a property?

[BenMorel]

I would suggest to revisit this issue, here's the kind of mess it creates:

https://github.com/doctrine/dbal/blob/dbd94c9d591e3d6f980f3a077cfa5b64c600b6d7/lib/Doctrine/DBAL/Query/QueryParts.php#L70

phpcs asks us to convert this code:

public function __clone()
{
    (...)

    if ($this->where !== null) {
        $this->where = clone $this->where;
    }

     if ($this->having !== null) {
        $this->having = clone $this->having;
    }
}

to:

public function __clone()
{
    (...)

    if ($this->where !== null) {
        $this->where = clone $this->where;
    }

    if ($this->having === null) {
        return;
    }

    $this->having = clone $this->having;
}

Which kills consistency & readability in this specific case.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.