Skip to content

Conversation

@joaorobertopb
Copy link

  • Use ternary operator
  • Early return
  • Remove unnecessary else statement

- Use ternary operator
- Early return
- Remove unnecessary else statement
@ntzm
Copy link
Contributor

ntzm commented Oct 6, 2017

Personally, I find the ternary operator to make this more confusing. I do like the early returns and removing the unnecessary else statements however.

@ludo237
Copy link

ludo237 commented Oct 6, 2017

Sometimes "less code" is not better. Readability should be a thing.

For instance this patch does not add anything in concrete. Why remove the else if there but not here? It doesn't make sense.

Those are just two examples, there are many of them.

@deleugpn
Copy link
Contributor

deleugpn commented Oct 6, 2017

#21526 (comment)

@ConnorVG
Copy link
Contributor

ConnorVG commented Oct 6, 2017

https://github.com/laravel/framework/pull/21554/files#diff-e526262c4637fb5e17fbe8f3788b97a9R114 This worries me; a lot of readability is lost with ternary and null-coalesce pairing.

I agree with a lot of the "dropping else in favour of early returns", though I feel it makes more sense to guard in most of these instances (such is if fail -> throw exception instead of if success else throw exception like here https://github.com/laravel/framework/pull/21554/files#diff-e265ae7dcca1de383f991786a8da27f4R20).

We could probably actually tidy up code in a few of these areas though, for instance https://github.com/laravel/framework/pull/21554/files#diff-df089e9b38d0b0f9f95fd3b2275744afR70 could easily just be return "$rootNamespace\Feature"

@taylorotwell
Copy link
Member

It's hard for me to merge these types of PRs. Some of them could be subtle behavior breaks. The Memcached one in particular sticks out as passing $null like that could be there for a reason because of some quirk in the API.

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.

6 participants