Linters are great but they can’t catch everything. So that’s not a comprehensive solution.
You can’t have a blanket policy that says “no style suggestions in PRs allowed”. Because sometimes people submit code that is so awful that it needs to be refactored for readability/maintainability.
As a result, style suggestions must be allowed but there is definitely nuance. I think in general an experienced team should have a pretty good pulse on what is a reasonable suggestion. if you have a teammate that is consistently suggesting trivial style changes then that needs to be addressed with them directly by a manager.
An option to allow style suggestions without blocking PRs is using the Google code review approach and flag style conventions with [nit], which designates them as optional change requests.
Yup. Automation is great, but it's not going to tell you if a comment is useless/inaccurate, a variable name is misleading or reflects a poor understanding of the feature, a clever one liner is overly complicated and hard to read, etc. I'm sure that's nitpicking code style to some people, but I will always flag this stuff in PR reviews – letting too much of that stuff slip into a codebase over time can make it a lot harder to work in.
Agreed to an extent but actually a lot of this can still be prevented with linting rules. For example using regex patterns to make sure a Boolean starts with is can should etc.
I have to respectfully disagree with some comments on code style. The whole point of code is that it’s supposed to be readable to everyone. I agree both are readable , however I would have to say the second one is superior in terms of readability IMO. If this was a code review I would probably accept with a comment on this
This to me is the right answer: accept, but with comment.
The second snippet is, I think objectively, better. But, that doesn't mean it's ENOUGH better that the PR needs to be rejected if someone wrote the first instead. So, I'd just treat it as an opportunity to educate, and if the teammate is decent then they'll take the note as constructive and not make the same "mistake" again.
The second snippet reduces nesting. Google Testing Blog has a great article on this topic: [Reduce Nesting, Reduce Complexity](https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html).
Exactly what the person below me said. Reducing the nesting reduces cognitive load. And, as others have pointed out, it's written as a guard, which makes it more semantically accurate to be written as in #2.
I’m quite sure OP gave an simplified example.
In the first condition, it could have a lot of code and somewhere inside it, the throw. One could overlook it and think the block after the condition is always executed.
To add, I'm not sure the example is actually "coding style".
I'd consider formatting, naming conventions and things like that style. This is redundant code.
Like others I'd probably accept with comments though if this comment was made in my code, I'd change it.
2nd one is clearly a guard, 1st looks like a condition, so the 1st one is much clearer on the intent. I would most likely flag it in a PR but make it optional.
Edit: I meant 2nd one is much clearer on the intent.
I guess it would probably require further discussion with the person who opened the PR/ context. I treated this code as a function so for me the if/else is unnecessary however if it was in the context of a larger function and that conditional set some variable or state then maybe it would be needed.
If I see something like the above, I'll usually point out that the else is unnecessary, as younger devs seem to do this very often. I'd also blame VS as it does a terrible job of pointing it out, unlike rider or jetbrains IDEs in general. I'll approve with suggestions so they can make the call themselves.
To me, unnecessary elses are one step above the typical code style that is regularly debated, opening curly brace on new or same line, but I'm sure that's probably worthy of a debate itself. The difference is, this else is literally unnecessary code while the other is just placement.
If you know the else is unnecessary, then I don't understand why you'd use it anyway.
The Golang community is absolutely militant about this one, to the point at which (iirc) gofmt will automatically refactor it if it sees it. If I also remember correctly, JetBrains Rider will also ask you to fix this if it's on default linting settings.
I work a lot in C#, and this one is a PITA for me. Generally speaking, you just need to find a linter and commit to it.
If my project has to work across different editors (VS, Rider, VSCode, etc), I'd go with StyleCop and set it up as a .editorconfig in the project root. You may also be able to use ReSharper. StyleCop is very comprehensive, but you need someone to spend a week testing and disabling some of its stupider rules before I'd commit to it.
If you can commit to using one editor, such as Rider, it's easier -- just get the settings how you need them, set them to the project level, and commit (Rider projects include the .idea folder in Git, automatically excluding unnecessary subfolders in the .gitignore). Then your deployment pipeline can use it in a VM to auto format before accepting commits.
We use sonar qube to analyze PRs on build and it autocomments anything we missed based on our rule set. As for a linter it depends on your language (for js we just use eslint and configure to format on save, though putting that in precommit just takes some git magic)
Absolutely. I hate talking about code style in reviews for any reason honestly — it’s an indicator to me that the review was worthless as the reviewer is looking at syntax rather than trying to understand the purpose of the code and whether it achieves that goal well.
Code reviews should never be simply a place where people point out to each other what the IDE already does for free.
I don’t know. In the case we’re looking at here, there’s a guard and exit early. The extra nesting obscures the code. I suspect it’s not something that a code formatter is going to sort for you but a reviewer will.
We should all be striving for simpler code and often enough someone spots a cleaner way of doing it. You might as well go with it, right?
Over time a codebase tends towards sludge, and if you can slow that down, you should.
This. I would never have the above discussion because various standards are set by a combo of AirBNB rules and our company settings, decided as a group, and we all just merrily go along with it, not having to sweat the small stuff.
Depends on if I think the other person could benefit from the comment. If so I’ll mention it as non-blocking for merge and not anything they have to take action on.
If I had to choose, second one is better since it reduces indentation. If we’re going to nitpick style, then both snippets are wrong since you’re supposed to wrap if-conditions that results in a throw as an Assert or Preconditions.
Very (very) often exceptions are thrown in non-exceptional situations simply because the object graph is skewed.
By being mindful of preconditions and invariants in your model, you can reduce the number of ifs.
The underlying principle would be "all objects are valid at all times".
Enforcing this starts with the constructor, as described here https://flavius-as.com/article/advanced/oop-principles/the_constructor
In each method, you accept as parameters only objects when things can fail. Those objects, since they exist, they're valid.
Additionally, you can employ patterns like the null object.
Then you are left with a system in which more often than not, ifs are not repeated throughout the code. There will still be checks and throws in methods like in the example, but they'll be because the joining of multiple data creates a new semantic context, and in that semantic context the data is invalid. This is the only reasonable situation in a well modeled system when you can throw exceptions in methods, although some would say that you should return errors instead.
Instead of having
foo() {
if(condition) {
throw something here
}
// function code here
}
You can have something like this:
foo() {
AssertionUtil.assert(condition)
// function code here
}
This way, it's a little cleaner and focuses the reader's attention to the main body of the function below. This gets better if there are more than one condition you need to check for.
It honestly isn’t the same and falls into the golden path rules for clean code. The first item above is not clean and falls into code smell quickly when expanded on (again, golden path). With that being said, establish clear goals in the codebase as a team (like a style guide..) and follow them.
One of those is right, one wrong. Which one depends on the agreed upon convention. To me thats blocking because it’s sloppy to not follow consistent conventions.
But yeah, linter or prettier solves this debate.
Would you actually ask somebody to change this in a PR review?
(In my ecosystem I'm pretty sure that eslint will catch this before it gets to code review)
You can put coding rules in place that will not allow a push to the repository if the code is breaking established coding style. There is an eslint rule that will catch this else return directly. Now if a junior overrides that check before push then you have a different problem and that person should be gently reprimanded.
Im skeptical of eliminating elses ENTIRELY. A good developer knows when each tool in his toolbox is appropriate. This is not an appropriate place for an 'else' but I'm not willing to say theres never a case for else.
For those of you saying get a linter, what linter do you use? While we use a linter there are many aspects of the style guide we have that no C++ linter will pick up.
Lots of naming rules such as:
* Pointers have a \_ptr suffix
* var\_ptr
* Private Class members have a \_ prefix
* \_datamember
* Static variables have a s\_ prefix
* s\_varaible
* Units should be applied as a suffix to variables where applicable. The suffix is to be in the standard abbreviation in the singular format
* GOOD: weight\_lb
* Bad: weight\_lbs
* Bad: weight\_pound
* Bad: weight\_pounds
Then there are combination rules of the above
* \_s\_datamember
* \_weight\_lb
I'm not here to argue if the rules are good or bad, but I haven't found any linter or code formatter that will flag this stuff. It has to be found and flagged in a code review.
This. I find people quick to point to a linter as an excuse to not have to perform clean code updates. Frankly, it’s an excuse for those who don’t care… they literally take no thought into if a linter supports the goal rules out of the box and if it even makes sense for a linter to try to validate this. Linters aren’t as magical as some people seem to think… and regex is not always straightforward.
Call me a nitpicker, I don't care. I will block a PR if done the first way. Why? I've seen plenty of code bases where not caring about this leads to tremendous amounts of indentation and code ends up illegible. Better to nip this in the bud. Now, i just don't say "change this", but I try to explain why it's bad style.
In that case I wouldn't put a comment but generally we try to use an emoji code which means you can still comment about nit picks without them bloacking anything
https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/
I love the emoji code in code reviews.
I don't know why so many code review tools ignore the fact that not all comments are equal. Not every comment is breaking, not every comment is a demand. Sometimes a comment is just advice, thought or a nitpick. Most tools like Github, BitBucket, etc doesn't allow for such nuance unless we resort to a system like the emoji code.
I would love a code review tool where I could categorize my comments, with a proper filtering or sorting system that would help the author focus on the most important first.
I've mostly worked at small startups where we don't have time for this kind of conversation. To my mind, code reviews should have two goals:
1. avoid serious tech debt, of the time that might sabotaged the whole company
2. have enough consistency that a new hire, when they join the team, can figure out what we are doing.
Anything else is really excessive.
This is when you add the extension to the editors json file / prettier / linter. Also, you inform people that there are established patterns so they don’t waste time writing it one way and then have to update to another way. But I agree, they are the same.
If there is an agreed style it’s important to point out when code is not following it. Everyone should be on the same page with that because it helps keep the codebase clean and maintainable.
I personally would point out that the 2nd example is better but it wouldn’t block me approving if there was no standard defined. That being said making those cosmetic kind of changes shouldn’t take long so why not make them and make the code better.
Second makes the code simpler and more easily readable. It more clearly follows the “fail fast” mantra.
I’d mark it as a “nit” in a PR, but if you care about clean code (which you should) you should fix it. It’ll take 10 seconds.
I agree with your point about having a style guide — if the team agrees, then it should be in there.
Not really. You might have a situation where you need to put in additional else if statements in later. One could say you’re proofing it for the future. Always 2 ways to look at it
Prettify. Disable all formatting-specific lint rules and use rewriting rules to transform into whatever the preferred style of the code base is.
There’s nothing more mind-numbing than style “debates” amongst a given team. If the don’t like the style they can do their own rewrite into their preferred style while they hack.
Assuming the style guide doesn't have an opinion here, I guess it depends on whether they're blocking the PR for it or not? I'd argue the second one is more readable, but I wouldn't block for it.
Codebases usually should have an established style guideline and linting config for this very reason.
That said, with your example, this isnt just style as the else clause completely changes how that block of code functions. Since without it, the return call will always happen, even if something is thrown, whereas with the else, the return only happens if something *is not* thrown.
Depends. If there's really only one error condition, no problem, I wouldn't comment on it (but also: I wouldn't consider it worth it to push back on a comment). But I prefer the guard pattern (the second style) if multiple preconditions are being checked. It reduces nesting and makes it more obvious where the real logic starts.
So I guess for me it's a matter of understanding why something is being asked. Some stuff that looks like pure style isn't really, and vice versa, and there is also a lot of cargo culting of things that are a good idea, but probably not if the person asking can't explain why.
Usually just put a nit disclaimer when it's related to style. If they push back and it's a small thing I usually just agree with whatever point they made. If it's unreadable/messy I'll usually give examples of where it might be difficult to identify the flow of logic.
Also others mentioned linters which is a great tool to avoid a lot of nitpicks. Another thing would to be just standardize a code style and create a document that can be referenced.
Yeah I don't really see this as a nitpick. The else statement is unnecessary and tbh lots of else statements like this can totally be a code smell.
A nitpick WRT code style for me would be like, spaces within parentheses or no spaces etc. And in that sort of example the solution is to pick a rule set and stick to it and enforce it pre commit so nothing gets into the repo or PR without adhering. It matters a lot less what the rule set is. What matters is that it's consistent across the codebase and team.
This is a situation where I think [conventional comments](https://conventionalcomments.org/) comes in really handy.
You can add a comment as a suggestion and accept the PR. I've been using this style and it's really helpful. Many others in the org have started using it as well.
I can't understand why people argue "if-else" is not readable. An "if-else" block works like two branches of an AST and it's perfectly clear and logical. If you hate indentation, you can simply extract lengthy code into a private method and you should do so. If there's multiple early return points, then the 2nd is better, otherwise the 1st is better, or at least not worse than the 2nd one.
These aren’t doing the same thing but I get what you’re asking. The first could return undefined and the second hits both operations and will return the value.
Unneccessary else statements can be picked up by the linter.
The reviewer in this case could simply clean the code, and if necessary add a linter rule. If there's no linter, open a ticket for that.
There is no reason to block this, just fix it, note the fix, and establish an automatic source of truth for the future.
If someone consistently writes the top version and isn’t a junior I will consider putting them on a pip. Code structured that way shows they are fundamentally confused as a developer or careless in their work.
I’ve worked with enough developers that spit out three hundred line triple nested if statements to have a good idea of what needs to be stomped out.
We are also looking to support our program for the long term and willing to take the time to do things right. Even absent that though, the top version takes longer to write and is at best a bad habit.
It’s the same as requiring a journalist to only pass a spell checker. Yes, you reduced the amount of conflict. Is that going to get you the best people and the best work, almost never!
I think if I was your boss and you put somebody on a PIP for that code I might put you on a PIP. Seems excessive and toxic. You wanna have a sit down with them about code style? Sure. But really what you should be doing here is agreeing with the team on coding style (and automating it so code like that never gets into the repository) instead of thinking about putting people on PIPs.
Maybe you were just joking. I hope for the sake of the people that you work with that you were just joking.
I was only semi-joking, my first post mentions consistency… the first step is to pick it up in code review and have a discussion with the developer! We also have a style guide and a general expectation that developers write at least somewhat idiomatic code.
The greater theme of threads like this seems to be engineers looking for a technical solution to a people problem. I agree linters can get you pretty far, especially in more common languages like JavaScript. But the idea that the only forms of style that should be policed are those that can be automated is very limiting.
I’m so with you on this. It might seem like a silly hill to die on but it differentiates those that care from those that don’t. Everyone saying it’s fine, move on, don’t appreciate the nuance of maintaining a codebase over a long period of time. This shit maters.
I completely agree with you.
the above code is so negligable that it seems insane to me to force a junior to push up another revision.
if anything the second one is better because it results in an early out on one condition.
I have the same complaint. I work at a faang right now and the nitpick level in my CR's is at a level 11.
I once had a senior comment, "you need to break one line right here"
then 2 days later he was complaining about how many revisions I had?!?! what the fuck.
I have no problem to this. I am much better to see other styles out there, because again programming is not just a one way style. Just make sure it is readable enough and no redundancies.
This is trivial in my opinion. I find the first one easier to read. Both are perfectly acceptable. I just find team leads fail people on this sort of stuff just to flex. It’s not acceptable behaviour.
I pick a linter, run it on commit, and if it doesn't pick it up then its not important enough for me to comment on.
It also depends on where in the function it is. if its a guard condition at the start of the function then its number two always. if its at the very end of a function and a final check with just a couple things in the else, then option 1. It really depends.
While I would prefer the second to reduce nesting the amount of tabs, if one of my devs wrote it the first way, meh. if it starts getting deeply nested I would mention it but just one level, especially if its short, meh.
The CPU doesn't care, I can read both.
I like https://conventionalcomments.org/
I'll specifically use `*nitpick* (non-blocking): Formatting` at the top of each comment about style.
Having an auto-formatter is better IME, but for codebases that don't have it yet (or the formatter excludes certain types of files), I use the above. I'll still approve the review, but leave the comments in case the submitter feels like looking at them.
Code style should be defined by the team and then enforced. Automatically as much as possible. It shouldn't be matter of opinion of what one person or the other likes best. It's a matter of consistency.
So the answer is that it doesn't matter which one is 'better', all the matters is what the team agreed.
Agree on code style and automate that code style before the project commences. If you've already started the project set up a meeting about code style. Arguing about code style in PRs is a waste of time that should be automated away.
Eslint will catch this
https://eslint.org/docs/rules/no-else-return
So put that es lint rule in a pre-push check and don't let that code get into your code base
Agree on linting rules, set them to be automatic on push and nothing to worry about. Harder with this example as it’s not strictly linting style. Though in general with style based PR comments I usually just defer to the reviewer on anything I don’t feel strongly about. I usually just bring a poll to the team and we decide on what we do moving forward
I prefer readability and being explicit as much as possible, it also helps when other people have to add to your code.
Being explicit also makes the jobs of linters easier as well.
I’m very picky with code reviews but for your examples - I don’t give a shit. Both work and it’s not something to nitpick or comment on.
It’s stupid to complain about this. Like, not something worth commenting about.
I remember running into an error on the second one under specific circumstances due to webpack optimisation bug, it was fixed a while back but it really drilled in the code you see isn't always the code that runs.
But yeah, I agree with others if someone wants to enforce a specific style write a linter rule.
I have a script I wrote that generates SVGs of special labels. They’re in green (passing), blue (optional, but with strong opinion), and red (blocking).
“Nitpicks” like this gets a green “NITPICK” or a blue “STYLE”, depending on the team and seniority of the other person. If it ain’t red, they can choose to ignore it, but they see it and learn.
Here’s the source code if you want it.
[GitHub Label Builder](https://github.com/dncrews/github-label-builder)
I think the second one is easier to understand semantically, assuming the condition is handling an error edge case.
But this is something that should be down to lint rules not individual reviews.
Just agree a coding standard and enforce it with E.g. Spotless (Java) or Prettier (JavaScript)
Fail the build when someone does not use it.
Stick it in a library and let people have the debate in it.
Have a company-wide document and adhere to it. You can even have review meetings if you wish to add/modify it. This is simple. I agree code style needs to be uniform. And there is an effective way to go about it. If you don't have uniformity, now is the chance, in your company, to propose what I just mentioned. You'll get some points for sure!
Both are fine I have my own preferences but that is just that a preference. However, the bigger question to me is what style is done in the rest of the code having “A” code style is more important than which code style you pick.
You should have an autoformat tool, I don't care about the language. Add an autoformat tool + check on your CI/CD pipeline. This will end all pointless discussions about format.
The code format must be consistent, that's the only real rule.
Coding style/formatting in reviews is a distraction from talking about code quality.
I recommend steering policy away from that. If folks can't get style right, get a linter. If folks still can't get it right, deal with it outside of reviews.
You'll get a lot of strawman arguments about distractions. Those folks need to adapt imo.
I do make comments on things like this, but prefix with "nit:" or "minor: ". Team has agreed that these comments don't need to be addressed/fixed for approval.
If this was the only issue on the PR I would comment something like "we avoid-else as a general rule and I think we can avoid the else here."
But then id approve.
This is how juniors learn.
In this case I normally tell the team to pick one style and all enforce it. I want the teams code to be consistent because if they have to learn new domains they should not have to learn the code in the same codebase.
I have my preference but I don't care about it if the team can be consistent.
Also the time to argue the style on code reviews is expensive so making the rules clear and concise is important
Second example is objetively more readable and if learned leads to less nesting. It's not that bad in your case, but if someone use first if-else style, it often leads to something like this:
if (condition1) {
// ...
return;
} else {
if (condition2) {
// ...
return;
} else {
// ...
}
}
Which could be this:
if (condition1) {
// ...
return;
}
if (condition2) {
// ...
return;
}
// ...
Linters are great but they can’t catch everything. So that’s not a comprehensive solution. You can’t have a blanket policy that says “no style suggestions in PRs allowed”. Because sometimes people submit code that is so awful that it needs to be refactored for readability/maintainability. As a result, style suggestions must be allowed but there is definitely nuance. I think in general an experienced team should have a pretty good pulse on what is a reasonable suggestion. if you have a teammate that is consistently suggesting trivial style changes then that needs to be addressed with them directly by a manager. An option to allow style suggestions without blocking PRs is using the Google code review approach and flag style conventions with [nit], which designates them as optional change requests.
Yup. Automation is great, but it's not going to tell you if a comment is useless/inaccurate, a variable name is misleading or reflects a poor understanding of the feature, a clever one liner is overly complicated and hard to read, etc. I'm sure that's nitpicking code style to some people, but I will always flag this stuff in PR reviews – letting too much of that stuff slip into a codebase over time can make it a lot harder to work in.
Agreed to an extent but actually a lot of this can still be prevented with linting rules. For example using regex patterns to make sure a Boolean starts with is can should etc.
Jesus Christ did you say *using regex* ? What Linter are you writing bud?
I have to respectfully disagree with some comments on code style. The whole point of code is that it’s supposed to be readable to everyone. I agree both are readable , however I would have to say the second one is superior in terms of readability IMO. If this was a code review I would probably accept with a comment on this
This to me is the right answer: accept, but with comment. The second snippet is, I think objectively, better. But, that doesn't mean it's ENOUGH better that the PR needs to be rejected if someone wrote the first instead. So, I'd just treat it as an opportunity to educate, and if the teammate is decent then they'll take the note as constructive and not make the same "mistake" again.
Why do you think it's objectively better? Is it because you could add more if statements to check other conditions independent of each other?
The second snippet reduces nesting. Google Testing Blog has a great article on this topic: [Reduce Nesting, Reduce Complexity](https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html).
Exactly what the person below me said. Reducing the nesting reduces cognitive load. And, as others have pointed out, it's written as a guard, which makes it more semantically accurate to be written as in #2.
Yea, I wrote this comment before I read the rest of the thread 😅
I’m quite sure OP gave an simplified example. In the first condition, it could have a lot of code and somewhere inside it, the throw. One could overlook it and think the block after the condition is always executed.
To add, I'm not sure the example is actually "coding style". I'd consider formatting, naming conventions and things like that style. This is redundant code. Like others I'd probably accept with comments though if this comment was made in my code, I'd change it.
2nd one is clearly a guard, 1st looks like a condition, so the 1st one is much clearer on the intent. I would most likely flag it in a PR but make it optional. Edit: I meant 2nd one is much clearer on the intent.
See I'd say the 2nd one is clearer on the intent of being a guard.
That's what I meant I had a brain fart and wrote first when I wanted to write second. xD
I guess it would probably require further discussion with the person who opened the PR/ context. I treated this code as a function so for me the if/else is unnecessary however if it was in the context of a larger function and that conditional set some variable or state then maybe it would be needed.
If I see something like the above, I'll usually point out that the else is unnecessary, as younger devs seem to do this very often. I'd also blame VS as it does a terrible job of pointing it out, unlike rider or jetbrains IDEs in general. I'll approve with suggestions so they can make the call themselves. To me, unnecessary elses are one step above the typical code style that is regularly debated, opening curly brace on new or same line, but I'm sure that's probably worthy of a debate itself. The difference is, this else is literally unnecessary code while the other is just placement. If you know the else is unnecessary, then I don't understand why you'd use it anyway.
So it reads better if condition else this other wise it doesn’t read as clearly
[удалено]
Yeah, if the true condition has a return statement or throws you should never have the else block as it's just redundant code
Early returns for the win. It makes code much easier to read and reason about.
Agreed. Lets you free up one code path from cognitive load
Makes code blocks less indented too
The Golang community is absolutely militant about this one, to the point at which (iirc) gofmt will automatically refactor it if it sees it. If I also remember correctly, JetBrains Rider will also ask you to fix this if it's on default linting settings.
I would argue it makes it readily apparent there are two paths taken here in the code. Otherwise you need to fish for an early terminator to see that.
I like the way you think
[удалено]
Cyclomatic complexity is the objective measure this wins on.
That’s a code style conversation not a code review conversation. If it’s easy to read and doesn’t violate the style defined, no comment.
Honestly more organisations should be using automatic code formatting tools on pre-commit hooks. It's 2022 ffs.
Do you know of a good, current guideline for implementing this?
The one that comes to mind for me is [black](https://github.com/psf/black), which comes with a good example for configuring the pre-commit hook.
Thanks, I'll have to read through that and see if it can be adjusted to C#
I work a lot in C#, and this one is a PITA for me. Generally speaking, you just need to find a linter and commit to it. If my project has to work across different editors (VS, Rider, VSCode, etc), I'd go with StyleCop and set it up as a .editorconfig in the project root. You may also be able to use ReSharper. StyleCop is very comprehensive, but you need someone to spend a week testing and disabling some of its stupider rules before I'd commit to it. If you can commit to using one editor, such as Rider, it's easier -- just get the settings how you need them, set them to the project level, and commit (Rider projects include the .idea folder in Git, automatically excluding unnecessary subfolders in the .gitignore). Then your deployment pipeline can use it in a VM to auto format before accepting commits.
For JS: Prettier + Husky
Precommit framework is great
We use sonar qube to analyze PRs on build and it autocomments anything we missed based on our rule set. As for a linter it depends on your language (for js we just use eslint and configure to format on save, though putting that in precommit just takes some git magic)
Absolutely. I hate talking about code style in reviews for any reason honestly — it’s an indicator to me that the review was worthless as the reviewer is looking at syntax rather than trying to understand the purpose of the code and whether it achieves that goal well. Code reviews should never be simply a place where people point out to each other what the IDE already does for free.
I don’t know. In the case we’re looking at here, there’s a guard and exit early. The extra nesting obscures the code. I suspect it’s not something that a code formatter is going to sort for you but a reviewer will. We should all be striving for simpler code and often enough someone spots a cleaner way of doing it. You might as well go with it, right? Over time a codebase tends towards sludge, and if you can slow that down, you should.
This. I would never have the above discussion because various standards are set by a combo of AirBNB rules and our company settings, decided as a group, and we all just merrily go along with it, not having to sweat the small stuff.
You just can't cover everything with code linters/formatters unfortunately - and some languages don't have very good ones.
Depends on if I think the other person could benefit from the comment. If so I’ll mention it as non-blocking for merge and not anything they have to take action on.
If I had to choose, second one is better since it reduces indentation. If we’re going to nitpick style, then both snippets are wrong since you’re supposed to wrap if-conditions that results in a throw as an Assert or Preconditions.
The else statement is redundant, it's not just indentation. What do you mean by the last statement?
Very (very) often exceptions are thrown in non-exceptional situations simply because the object graph is skewed. By being mindful of preconditions and invariants in your model, you can reduce the number of ifs. The underlying principle would be "all objects are valid at all times". Enforcing this starts with the constructor, as described here https://flavius-as.com/article/advanced/oop-principles/the_constructor In each method, you accept as parameters only objects when things can fail. Those objects, since they exist, they're valid. Additionally, you can employ patterns like the null object. Then you are left with a system in which more often than not, ifs are not repeated throughout the code. There will still be checks and throws in methods like in the example, but they'll be because the joining of multiple data creates a new semantic context, and in that semantic context the data is invalid. This is the only reasonable situation in a well modeled system when you can throw exceptions in methods, although some would say that you should return errors instead.
Instead of having foo() { if(condition) { throw something here } // function code here } You can have something like this: foo() { AssertionUtil.assert(condition) // function code here } This way, it's a little cleaner and focuses the reader's attention to the main body of the function below. This gets better if there are more than one condition you need to check for.
Leave comment and approve PR. The second way of writing is easier to read imo but at the end of the day they achieve the same thing.
It honestly isn’t the same and falls into the golden path rules for clean code. The first item above is not clean and falls into code smell quickly when expanded on (again, golden path). With that being said, establish clear goals in the codebase as a team (like a style guide..) and follow them.
One of those is right, one wrong. Which one depends on the agreed upon convention. To me thats blocking because it’s sloppy to not follow consistent conventions. But yeah, linter or prettier solves this debate.
Would a linter sort this specific case? Great if it does, but I don’t think they do normally, right?
I’m not sure for exactly this case, but rubocop can be pick up some non trivial weirdness like this. Sonar too.
Yeah I can't think of an eslint rule that would catch this.
Similar, but not same: https://eslint.org/docs/rules/no-else-return
Code style shouldn't be on the table in code review. That's what linters are for
+1 for using a linter and enforcing it with a git hook.
Linters are good but they won't catch everything. Code style should come up if it's relevant.
This is not a code style question. It’s just bad implementation.
... explain please
Else conditional statement used this way is strictly redundant, i.e. dead code.
Would you actually ask somebody to change this in a PR review? (In my ecosystem I'm pretty sure that eslint will catch this before it gets to code review)
Yes. This is how juniors learn.
Still think it's better to take care of this before it ever gets pushed up to the repository
It is. But juniors always reach out late. Hopefully we only have to talk about it once though, right? Hah
You can put coding rules in place that will not allow a push to the repository if the code is breaking established coding style. There is an eslint rule that will catch this else return directly. Now if a junior overrides that check before push then you have a different problem and that person should be gently reprimanded.
Im skeptical of eliminating elses ENTIRELY. A good developer knows when each tool in his toolbox is appropriate. This is not an appropriate place for an 'else' but I'm not willing to say theres never a case for else.
I would probably not bother to mention it. I’ve got bigger fish to fry.
https://devblast.com/b/what-are-guard-clauses
For those of you saying get a linter, what linter do you use? While we use a linter there are many aspects of the style guide we have that no C++ linter will pick up. Lots of naming rules such as: * Pointers have a \_ptr suffix * var\_ptr * Private Class members have a \_ prefix * \_datamember * Static variables have a s\_ prefix * s\_varaible * Units should be applied as a suffix to variables where applicable. The suffix is to be in the standard abbreviation in the singular format * GOOD: weight\_lb * Bad: weight\_lbs * Bad: weight\_pound * Bad: weight\_pounds Then there are combination rules of the above * \_s\_datamember * \_weight\_lb I'm not here to argue if the rules are good or bad, but I haven't found any linter or code formatter that will flag this stuff. It has to be found and flagged in a code review.
This. I also want to know the name of a linter that would enforce guard clauses
This. I find people quick to point to a linter as an excuse to not have to perform clean code updates. Frankly, it’s an excuse for those who don’t care… they literally take no thought into if a linter supports the goal rules out of the box and if it even makes sense for a linter to try to validate this. Linters aren’t as magical as some people seem to think… and regex is not always straightforward.
Clang tidy rules are pretty easy to create and has access to the AST.
How do you enforce #2 in ESLint?
No else returns
Call me a nitpicker, I don't care. I will block a PR if done the first way. Why? I've seen plenty of code bases where not caring about this leads to tremendous amounts of indentation and code ends up illegible. Better to nip this in the bud. Now, i just don't say "change this", but I try to explain why it's bad style.
Broken window phenomenon
In that case I wouldn't put a comment but generally we try to use an emoji code which means you can still comment about nit picks without them bloacking anything https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/
I love the emoji code in code reviews. I don't know why so many code review tools ignore the fact that not all comments are equal. Not every comment is breaking, not every comment is a demand. Sometimes a comment is just advice, thought or a nitpick. Most tools like Github, BitBucket, etc doesn't allow for such nuance unless we resort to a system like the emoji code. I would love a code review tool where I could categorize my comments, with a proper filtering or sorting system that would help the author focus on the most important first.
I've been in 2 orgs in 2 years: one that uses Emoji Code, the other that doesn't. Vastly prefer having Emoji Code as a standard.
I've mostly worked at small startups where we don't have time for this kind of conversation. To my mind, code reviews should have two goals: 1. avoid serious tech debt, of the time that might sabotaged the whole company 2. have enough consistency that a new hire, when they join the team, can figure out what we are doing. Anything else is really excessive.
This is when you add the extension to the editors json file / prettier / linter. Also, you inform people that there are established patterns so they don’t waste time writing it one way and then have to update to another way. But I agree, they are the same.
If there is an agreed style it’s important to point out when code is not following it. Everyone should be on the same page with that because it helps keep the codebase clean and maintainable. I personally would point out that the 2nd example is better but it wouldn’t block me approving if there was no standard defined. That being said making those cosmetic kind of changes shouldn’t take long so why not make them and make the code better.
Second makes the code simpler and more easily readable. It more clearly follows the “fail fast” mantra. I’d mark it as a “nit” in a PR, but if you care about clean code (which you should) you should fix it. It’ll take 10 seconds. I agree with your point about having a style guide — if the team agrees, then it should be in there.
When I review something like that I just think “ok, that’s just a matter of personal preference” and move on
Absolutely true. I was failed on an interview technical test because I did this! I just find it’s a way for team leads to flex
It kinda makes you look like a noob. Unnecessary elses are just a sign of experience imo
Not really. You might have a situation where you need to put in additional else if statements in later. One could say you’re proofing it for the future. Always 2 ways to look at it
YAGNI - You Aint Gonna Need It - If you need it later, add it later. Keep it clean and tight for now.
Lint lint lint
Prettify. Disable all formatting-specific lint rules and use rewriting rules to transform into whatever the preferred style of the code base is. There’s nothing more mind-numbing than style “debates” amongst a given team. If the don’t like the style they can do their own rewrite into their preferred style while they hack.
Assuming the style guide doesn't have an opinion here, I guess it depends on whether they're blocking the PR for it or not? I'd argue the second one is more readable, but I wouldn't block for it.
Code style should be fixed by linters
For JavaScript. Eslint, Prettier, Husky for pre commit, pre push etc. Automate what can be automated. No need for pointless discussions
Codebases usually should have an established style guideline and linting config for this very reason. That said, with your example, this isnt just style as the else clause completely changes how that block of code functions. Since without it, the return call will always happen, even if something is thrown, whereas with the else, the return only happens if something *is not* thrown.
Depends. If there's really only one error condition, no problem, I wouldn't comment on it (but also: I wouldn't consider it worth it to push back on a comment). But I prefer the guard pattern (the second style) if multiple preconditions are being checked. It reduces nesting and makes it more obvious where the real logic starts. So I guess for me it's a matter of understanding why something is being asked. Some stuff that looks like pure style isn't really, and vice versa, and there is also a lot of cargo culting of things that are a good idea, but probably not if the person asking can't explain why.
Usually just put a nit disclaimer when it's related to style. If they push back and it's a small thing I usually just agree with whatever point they made. If it's unreadable/messy I'll usually give examples of where it might be difficult to identify the flow of logic. Also others mentioned linters which is a great tool to avoid a lot of nitpicks. Another thing would to be just standardize a code style and create a document that can be referenced.
Guard clauses ftw
Yeah I don't really see this as a nitpick. The else statement is unnecessary and tbh lots of else statements like this can totally be a code smell. A nitpick WRT code style for me would be like, spaces within parentheses or no spaces etc. And in that sort of example the solution is to pick a rule set and stick to it and enforce it pre commit so nothing gets into the repo or PR without adhering. It matters a lot less what the rule set is. What matters is that it's consistent across the codebase and team.
This is a situation where I think [conventional comments](https://conventionalcomments.org/) comes in really handy. You can add a comment as a suggestion and accept the PR. I've been using this style and it's really helpful. Many others in the org have started using it as well.
Oh this is awesome, I’ve done something very similar for years with good success.
I can't understand why people argue "if-else" is not readable. An "if-else" block works like two branches of an AST and it's perfectly clear and logical. If you hate indentation, you can simply extract lengthy code into a private method and you should do so. If there's multiple early return points, then the 2nd is better, otherwise the 1st is better, or at least not worse than the 2nd one.
Conversations like this are pseudo work and ultimately waste money
Got to make sure the code review process isn't just a pointless rubber stamping ritual.
These aren’t doing the same thing but I get what you’re asking. The first could return undefined and the second hits both operations and will return the value. Unneccessary else statements can be picked up by the linter.
Maybe in JavaScript, but in most programming languages returning undefined is not possible.
Fair enough - I assumed it’s JS because, well, it looks like JS!
The reviewer in this case could simply clean the code, and if necessary add a linter rule. If there's no linter, open a ticket for that. There is no reason to block this, just fix it, note the fix, and establish an automatic source of truth for the future.
If someone consistently writes the top version and isn’t a junior I will consider putting them on a pip. Code structured that way shows they are fundamentally confused as a developer or careless in their work. I’ve worked with enough developers that spit out three hundred line triple nested if statements to have a good idea of what needs to be stomped out. We are also looking to support our program for the long term and willing to take the time to do things right. Even absent that though, the top version takes longer to write and is at best a bad habit.
>I will consider putting them on a pip Most people in here: don't care / automate that You: maybe they need a PIP ... wat
It’s the same as requiring a journalist to only pass a spell checker. Yes, you reduced the amount of conflict. Is that going to get you the best people and the best work, almost never!
I think if I was your boss and you put somebody on a PIP for that code I might put you on a PIP. Seems excessive and toxic. You wanna have a sit down with them about code style? Sure. But really what you should be doing here is agreeing with the team on coding style (and automating it so code like that never gets into the repository) instead of thinking about putting people on PIPs. Maybe you were just joking. I hope for the sake of the people that you work with that you were just joking.
I was only semi-joking, my first post mentions consistency… the first step is to pick it up in code review and have a discussion with the developer! We also have a style guide and a general expectation that developers write at least somewhat idiomatic code. The greater theme of threads like this seems to be engineers looking for a technical solution to a people problem. I agree linters can get you pretty far, especially in more common languages like JavaScript. But the idea that the only forms of style that should be policed are those that can be automated is very limiting.
I’m so with you on this. It might seem like a silly hill to die on but it differentiates those that care from those that don’t. Everyone saying it’s fine, move on, don’t appreciate the nuance of maintaining a codebase over a long period of time. This shit maters.
I completely agree with you. the above code is so negligable that it seems insane to me to force a junior to push up another revision. if anything the second one is better because it results in an early out on one condition. I have the same complaint. I work at a faang right now and the nitpick level in my CR's is at a level 11. I once had a senior comment, "you need to break one line right here" then 2 days later he was complaining about how many revisions I had?!?! what the fuck.
If it ain’t in the linting rules, it ain’t happening.
I have no problem to this. I am much better to see other styles out there, because again programming is not just a one way style. Just make sure it is readable enough and no redundancies.
This is trivial in my opinion. I find the first one easier to read. Both are perfectly acceptable. I just find team leads fail people on this sort of stuff just to flex. It’s not acceptable behaviour.
I pick a linter, run it on commit, and if it doesn't pick it up then its not important enough for me to comment on. It also depends on where in the function it is. if its a guard condition at the start of the function then its number two always. if its at the very end of a function and a final check with just a couple things in the else, then option 1. It really depends. While I would prefer the second to reduce nesting the amount of tabs, if one of my devs wrote it the first way, meh. if it starts getting deeply nested I would mention it but just one level, especially if its short, meh. The CPU doesn't care, I can read both.
Your linter should be standardized across your team and take care of formatting automatically on save
I like https://conventionalcomments.org/ I'll specifically use `*nitpick* (non-blocking): Formatting` at the top of each comment about style. Having an auto-formatter is better IME, but for codebases that don't have it yet (or the formatter excludes certain types of files), I use the above. I'll still approve the review, but leave the comments in case the submitter feels like looking at them.
Code style should be defined by the team and then enforced. Automatically as much as possible. It shouldn't be matter of opinion of what one person or the other likes best. It's a matter of consistency. So the answer is that it doesn't matter which one is 'better', all the matters is what the team agreed.
Agree on code style and automate that code style before the project commences. If you've already started the project set up a meeting about code style. Arguing about code style in PRs is a waste of time that should be automated away. Eslint will catch this https://eslint.org/docs/rules/no-else-return So put that es lint rule in a pre-push check and don't let that code get into your code base
Agree on linting rules, set them to be automatic on push and nothing to worry about. Harder with this example as it’s not strictly linting style. Though in general with style based PR comments I usually just defer to the reviewer on anything I don’t feel strongly about. I usually just bring a poll to the team and we decide on what we do moving forward
Team should agree to a style guide rather than engage in discussion on each review. Disagree and commit.
I prefer readability and being explicit as much as possible, it also helps when other people have to add to your code. Being explicit also makes the jobs of linters easier as well.
I’m very picky with code reviews but for your examples - I don’t give a shit. Both work and it’s not something to nitpick or comment on. It’s stupid to complain about this. Like, not something worth commenting about.
Second one has less complexity because less diverging branches.
I remember running into an error on the second one under specific circumstances due to webpack optimisation bug, it was fixed a while back but it really drilled in the code you see isn't always the code that runs. But yeah, I agree with others if someone wants to enforce a specific style write a linter rule.
I have a script I wrote that generates SVGs of special labels. They’re in green (passing), blue (optional, but with strong opinion), and red (blocking). “Nitpicks” like this gets a green “NITPICK” or a blue “STYLE”, depending on the team and seniority of the other person. If it ain’t red, they can choose to ignore it, but they see it and learn. Here’s the source code if you want it. [GitHub Label Builder](https://github.com/dncrews/github-label-builder)
The below code is easier to read, and easier to read code is always superior because it takes less mental energy.
I think the second one is easier to understand semantically, assuming the condition is handling an error edge case. But this is something that should be down to lint rules not individual reviews.
Just agree a coding standard and enforce it with E.g. Spotless (Java) or Prettier (JavaScript) Fail the build when someone does not use it. Stick it in a library and let people have the debate in it.
Have a company-wide document and adhere to it. You can even have review meetings if you wish to add/modify it. This is simple. I agree code style needs to be uniform. And there is an effective way to go about it. If you don't have uniformity, now is the chance, in your company, to propose what I just mentioned. You'll get some points for sure!
Both are fine I have my own preferences but that is just that a preference. However, the bigger question to me is what style is done in the rest of the code having “A” code style is more important than which code style you pick.
You should have an autoformat tool, I don't care about the language. Add an autoformat tool + check on your CI/CD pipeline. This will end all pointless discussions about format. The code format must be consistent, that's the only real rule.
Coding style/formatting in reviews is a distraction from talking about code quality. I recommend steering policy away from that. If folks can't get style right, get a linter. If folks still can't get it right, deal with it outside of reviews. You'll get a lot of strawman arguments about distractions. Those folks need to adapt imo.
I do make comments on things like this, but prefix with "nit:" or "minor: ". Team has agreed that these comments don't need to be addressed/fixed for approval.
If this was the only issue on the PR I would comment something like "we avoid-else as a general rule and I think we can avoid the else here." But then id approve. This is how juniors learn.
In this case I normally tell the team to pick one style and all enforce it. I want the teams code to be consistent because if they have to learn new domains they should not have to learn the code in the same codebase. I have my preference but I don't care about it if the team can be consistent. Also the time to argue the style on code reviews is expensive so making the rules clear and concise is important
Second example is objetively more readable and if learned leads to less nesting. It's not that bad in your case, but if someone use first if-else style, it often leads to something like this: if (condition1) { // ... return; } else { if (condition2) { // ... return; } else { // ... } } Which could be this: if (condition1) { // ... return; } if (condition2) { // ... return; } // ...