-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Conversation
👎 imo having the else makes the code easier to understand and read |
ok you advocate bad practices? |
no, I give my opinion based on my preferences. |
granted, but i still think is simpler to read this way, cleaner and less lines, and advocate a good practice |
When talking about cleaner ways, I'd prefer just one if (...) {
$response = ...;
} else {
$response = ...;
}
return $response; |
The Symfony codebase is using early returns without |
ok, 👍 then :) |
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. |
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) |
At least we should change the example to be consistent which what is actually the "standard" being used. |
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)); |
👍 ready to merge then 🔨 |
i guess this is not that important, closing @weaverryan |
I is important, code in the doc should follow symfony standards. |
yes but this is a change, don't want to change the whole documentation examples, so it should be a simple merge right? |
Yes, it's a simple merge |
No description provided.