Remove unnecessary else

[krolow]

only a minor change while i was reading this code, as it is returning in the previous if does not need the else...

[jimfb]

Eeeh, sure. I guess we can take this.

For future reference, we generally ask that minor fixes like this are thrown into a larger more meaningful commit - the reason being that there are trillions of these little fixes and constantly merging them would burry the meaningful changes in noise.

Before we can merge, you'll need to sign the CLA here: https://code.facebook.com/cla

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[gaearon]

I would be against taking it unless it fixes some issue. This obscures the blame/history information but doesn’t bring any changes. Additionally, we have many similar patterns in the codebase, and doing it only a single place doesn’t seem very consistent with the rest of the code, even if it reads easier in this particular case. That said I appreciate the contribution!

[krolow]

oh okay, yeah for sure it does not bring nothing new, but readability is always readability, and reading other files i could saw other places that might be able to improve readability. I do understand that does not make sense to merge that... but

do you think one PR to improve the readability for more than one file would be welcome or as it does not bring anything new/or/fix something it does not worth the effort? (i'd appreciate to work on that)

[gaearon]

Readability is important but in this particular case it’s somewhat subjective. Some people are more used to the return-early pattern, some people are used to the branching. In fact sometimes return-early can be a problem—for example, you might want to add some code in the beginning and in the end of the method that must be called consistently (e.g. performance measurements), and in this case you have to restructure the code to avoid the early returns. So I’m not saying they are bad, but they’re not universally better either 😄 .

Generally, improving readability is great when it’s a part of some larger body of work (e.g. fixing a bug). In a mature project like React, it is often more important to have very precise history of changes to the file. This is extremely useful for finding where the bugs were introduced. In this case, small changes like this distract the person looking for the bug because it’s not clear whether the change was harmless, or some small behavioral change was accidentally overlooked.

Even if everything is fine, it still changes the “blame” information on the particular lines so you no longer see the most recent actual change to the logic anymore. This is why big projects usually avoid large stylistic changes like adding/removing semicolons: they will remove the useful blame information from every file because they touch every line. Of course this isn’t what we’re dealing with here, but if we accepted this particular change, we’d have to choose where to draw the line, and it would be strange if we later rejected similar changes for no good reasons.

I hope this clarifies my thinking a bit. We want the codebase to be readable, but readability is often subjective whereas making small changes adds burden to people changing and debugging this code in the future.

[krolow]

hey @gaearon yeah it does make sense man, appreciate the feedback and got your point nice explanation ;)

[gaearon]

No problem, thank you for taking time to contribute!

[Bilge]

@gaearon Alternatively, you could just learn how to browse the blame history properly.

[gaearon]

@Bilge Can you please suggest any resources so I could learn it? Thanks!

[mikegreiling]

While it can be tedious to have to go back further into the history to use blame, I've never found changes like this to really impede my ability to learn where a bit of logic was added... it usually amounts to two or three extra clicks on github.

[Bilge]

Allowing "blames" to drive architectural decisions is a slippery slope that will eventually lead to bad decisions and equally bad code.

[jimfb]

@Bilge This PR is not an architectural decision/change. It's not even a functionality change. It's purely style, and it's not even objectively better; style is a matter of debatable opinion.