Skip to content
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

fixed if statement warnings #385

Closed
wants to merge 1 commit into from

Conversation

gherlein
Copy link
Contributor

No description provided.

@nickmooney
Copy link
Collaborator

I'd love to hear from other reviewers, but I am inclined not to merge this request. I am not fond of the bracket-less syntax -- rather than put it on one line, I would prefer to see the more verbose but far clearer bracketed syntax:

if (foo) {
    bar();
}

@wmarkow
Copy link
Contributor

wmarkow commented Nov 15, 2017

@nickmooney, I prefer the same way as you: to put brackets even for one line syntax. When you have more lines after if and no brackets then it could be like a trap (possibility of a bug because of lack of the brackets) and you need a little more time to process and find out what code will be executed in if statement.

I have the feeling that @gherlein followed the pattern: there were no brackets in the code before, so he didn't introduced them in his fix. I can understand this point of view.

@wmarkow wmarkow mentioned this pull request Dec 28, 2017
@Avamander
Copy link
Member

Fixed in 71ec8a1

@Avamander Avamander closed this Jan 17, 2020
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.

4 participants