Coding-standard: Feature request: isolated return statement

[Levivb]

Hey,

I've got a feature request which could improve overal code readability.

One of my colleagues came with the convention to place a newline before a return statement in a function (and closures/anonymous functions etc.) if the function body contains other logic to increase readability.

For example:

// This is fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);

    return $result;
}

// This is also fine:
function myFunction(): string
{
    return $result;
}

// This comment is fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);

    // Some explanation here
    return $result;
}

// This comment -although questionable- could be fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);
    // Some explanation here

    return $result;
}


// This is not fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);
    return $result;
}

// This comment is not fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);
    // Some explanation here
    return $result;
}

// This might not be fine.. bit weird to have a random newline laying around:
function myFunction(): string
{

    return $result;
}

// This is already caught by PSR2.Methods.FunctionClosingBrace.SpacingBeforeClose
function myFunction(): string
{
    return $result;

}

Let me know what you think :)

[Majkl578]

Some time ago we talked with @morozov about spacing before/around constructs like return, if, switch etc. So if we can expand/generalize this idea, it may be a good rule. :+1:

[morozov]

This is the rule I’m missing the most.

[kukulich]

@Majkl578 @morozov Please write the list :)

[morozov]

All PHP control structures besides require/include and declare (not sure why they are in that list):

• if-elseif/else if-else
• while
• do-while
• for
• foreach
• break
• continue
• switch
• return
• goto

Also,

• try-catch-finally
• throw
• yield

[morozov]

// This comment -although questionable- could be fine:
function myFunction(): string
{
    $result = $this->doSomeCode();
    $result = $this->doSomeMoreCode($result);
    // Some explanation here

    return $result;
}

I don't think it should be an exception. The comment should stick to the return. Once GitHub is back up, I'll try to find my previous comments on the topic and add more firmalized requirements for the rule to make sure we're all on the same page.

[morozov]

The previous conversations is somewhere deep in comments, so here are the requirements which come to mind:

1. A control structure should be surrounded by empty lines:

   $a = 1;
   
   if ($b > 0) {
      // do stuff
   }
   
   $c = 2;
   

2. Two ajacent blocks will share the empty line between them:

   if ($a > 0) {
      // do stuff
   }
   
   return;
   

3. The empty line is not needed before the block if it's the first statement in the parent block (same for the empty line after the last block)
   php if ($a > 0) { if ($b > 0) { // do stuff } }

4. Comments will be considered as belonging to the control structures which follow them:

   if ($a > 0) {
      // do stuff
   }
   
   // another condition
   if ($b > 0) {
      // do stuff
   }
   

5. Sets of control statements like do-while or try-catch (see the full list in https://github.com/slevomat/coding-standard/issues/472#issuecomment-420329126) are considered a single block and don't need empty lines within the block:

   if ($a > 0) {
      // do stuff
   } else {
      // do other stuff
   }
   
   do {
      // something
   } while (true);
   

[kukulich]

@morozov I need the last thing - the sniff name :)

[morozov]

@kukulich how about ControlStructures.RequireEmptyLinesAround?

[Majkl578]

I agree with the list above, except yield and throw.
Also case+return or case+break would imho look weird.

[morozov]

@Majkl578 could you provide the examples of what would look wrong based on the rules above?

[Majkl578]

My issues with those two are snippets like these:

try {
    // ...
} catch (E $e) {
    throw $e; // shouldn't need a blank line
}

foo() or throw new E();

function () : iterable {
    yield from a();
    yield from b();
};

function () : Generator {
    $a = yield a();
    $b = yield b();
};

[morozov]

try {
    // ...
} catch (E $e) {
    throw $e; // shouldn't need a blank line
}

Right, because it's the only statement in the block (#‌3).

foo() or throw new E();

Didn't think of this but yes. We can consider foo() part of the "visual block". So it still has to have empty lines around it:

foo() or throw new E();

bar() or throw new E();

function () : iterable {
    yield from a();
    yield from b();
};

function () : Generator {
    $a = yield a();
    $b = yield b();
};

Agree. However it make sense to require empty lines if the expression after yield is multi-line:

function () : iterable {
    yield [
        'foo' => 'a',
        'bar' => 'b',
    ];
// empty line required
    yield [
        'foo' => 'c',
        'bar' => 'd',
    ];
};

BTW, the same could be applied to anonymous function declarations:

$a = function () void {
    return;
};
// empty line required
$b = function () void {
    return;
};

[kukulich]

foo() or throw new E();

Is it valid PHP code? https://3v4l.org/SXRnv

[kukulich]

Implemented in https://github.com/slevomat/coding-standard/commit/93c582ce997e485f34e825f0950c042de980d35c

[github-actions[bot]]

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.