cutenode / good-first-issue

🖥 CLI for finding good first issues
736 stars 123 forks source link

Replace ternaries with normal if statements #141

Closed DanielRuf closed 4 years ago

DanielRuf commented 5 years ago

Ternaries are not clean code. Use normal if statements to handle this.

bnb commented 5 years ago

Can you provide context and reasoning for this change outside of "Ternaries are not clean code"? This feels more like a personal opinion rather than an objective fact.

DanielRuf commented 5 years ago

Such ternaries are also hard to read.

bnb commented 4 years ago

I see no reason to introduce this outside of personal preference. Going to close, but thank you for the time/effort!

DanielRuf commented 4 years ago

Not a personal preference but Clean Code.

bnb commented 4 years ago

I asked what "Clean Code" was before and you failed to answer, instead pivoting to a different reasoning. Repeating that same thing was never clarified probably isn't going to be a successful approach.

DanielRuf commented 4 years ago

I see no comment here from me regarding going into detail about Clean Code ;-)

This was probably another issue / project.

SonarQube, linting rules and other tools often propose if-else instead of a (longer) ternary.

https://stackoverflow.com/a/160295/753676

Either we reformat (newlines is often better than one one line) or we replace it with a better readable if-else ;-)

Sorry that I had not much more time to go into detail, too many other things todo.

bnb commented 4 years ago

Can you provide context and reasoning for this change outside of "Ternaries are not clean code"?

https://github.com/cutenode/good-first-issue/pull/141#issuecomment-521848605

DanielRuf commented 4 years ago

As I already wrote, I probably forgot to go into further detail ;-)

Such ternaries are also hard to read.

This is one of the reasons why ternaries are often bad. At least when they are only on one line ;-)

Would it be ok if I would make it multiline? Makes it easier to see both cases.

bnb commented 4 years ago

SonarQube, linting rules and other tools often propose if-else instead a (longer) ternary.

This project uses Standard, which does not enforce this rule. "Someone else prefers to do it another way" is not a compelling argument.

I won't be merging PRs around changing the ternary. There has still been no objective reason presented outside of personal preference.

DanielRuf commented 4 years ago

OK =)

DanielRuf commented 4 years ago

Just for reference: https://eslint.org/docs/rules/multiline-ternary

Would be easier to read imo. Just my two cents.

(But : and ? at the start of the lines is often better).