Skip to content

remove else and parenthesis unneeded #2884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

cordoval
Copy link
Contributor

@cordoval cordoval commented Aug 8, 2013

No description provided.

@wouterj
Copy link
Member

wouterj commented Aug 8, 2013

👎 imo having the else makes the code easier to understand and read

@cordoval
Copy link
Contributor Author

cordoval commented Aug 8, 2013

ok you advocate bad practices? :rage4:

@wouterj
Copy link
Member

wouterj commented Aug 8, 2013

no, I give my opinion based on my preferences.

@cordoval
Copy link
Contributor Author

cordoval commented Aug 8, 2013

granted, but i still think is simpler to read this way, cleaner and less lines, and advocate a good practice

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2013

When talking about cleaner ways, I'd prefer just one return statement like this:

if (...) {
    $response = ...;
} else {
    $response = ...;
}

return $response;

@stof
Copy link
Member

stof commented Aug 9, 2013

The Symfony codebase is using early returns without else. So this PR is making it consistent with the code

@wouterj
Copy link
Member

wouterj commented Aug 9, 2013

ok, 👍 then :)

@xabbuh
Copy link
Member

xabbuh commented Aug 9, 2013

Thanks for the clarification, @stof. Is it documented anywhere? I didn't find it in the CS and the first example in http://symfony.com/doc/current/contributing/code/standards.html doesn't follow this way.

@stof
Copy link
Member

stof commented Aug 9, 2013

I don't think it is written in the doc, but it is applied when reviewing code (it may be worth adding it in the doc)

@xabbuh
Copy link
Member

xabbuh commented Aug 9, 2013

At least we should change the example to be consistent which what is actually the "standard" being used.

@xabbuh
Copy link
Member

xabbuh commented Aug 9, 2013

if ('string' === $dummy) {
    if ('values' === $mergedOptions['some_default']) {
        return substr($dummy, 0, 5);
    }
    return ucwords($dummy);
} else {
    throw new \RuntimeException(sprintf('Unrecognized dummy option "%s"', $dummy));
}

This would match the standard?

Or should we use the same style to throw exceptions? Like this:

if ('string' === $dummy) {
    if ('values' === $mergedOptions['some_default']) {
        return substr($dummy, 0, 5);
    }
    return ucwords($dummy);
}
throw new \RuntimeException(sprintf('Unrecognized dummy option "%s"', $dummy));

@cordoval
Copy link
Contributor Author

cordoval commented Aug 9, 2013

👍 ready to merge then 🔨

@cordoval
Copy link
Contributor Author

i guess this is not that important, closing @weaverryan

@cordoval cordoval closed this Aug 25, 2013
@wouterj
Copy link
Member

wouterj commented Aug 25, 2013

I is important, code in the doc should follow symfony standards.

@wouterj wouterj reopened this Aug 25, 2013
@cordoval
Copy link
Contributor Author

yes but this is a change, don't want to change the whole documentation examples, so it should be a simple merge right?

@wouterj
Copy link
Member

wouterj commented Aug 25, 2013

Yes, it's a simple merge

@weaverryan
Copy link
Member

Hey Luis!

I've patched this into the 2.2 branch at sha: e4ecb50. Thanks for this change and for the good conversation around this - the coding standards will be clearer after #2886.

Thanks!

@weaverryan weaverryan closed this Aug 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants