"If someone has committed many crimes against the style guide in a PR, the reviewer should point them to the style guide..."
Everything a computer can do trivially should not be done by humans. There are plenty of tools available to validate code to all the standards you can dream up. And most languages allow you to even automatically fix style errors or even enforce it for compilation (thank you Go).
So instead of codifying it in a style guide document, codify it in config files for linting and autoformat tools instead. The defaults of those tools are often fine for what you need and exceptions can as easily be documented in them as well.
Btw, an added bonus of having a automation friendly standard for your codebase is that is can be easily converted in something you prefer reading. Prefer brackets below the condition instead of at the end? With one standard to convert to/from you can write a simple script to do that using a Git filter so every time you commit/checkout the transformation is automatically applied (https://git-scm.com/book/en/v2/Customizing-Git-Git-Attribute...).
Also one example on how to codify almost any style is using something like Sonarqube SSLR (https://docs.sonarqube.org/display/DEV/SSLR) where the code is converted into an abstract representation where comprehensible rule can be written for (instead of for example a bunch of regexes). But you really have to be deep into wanting to enforce every aspect of a style to dive into this one.
> Everything a computer can do trivially should not be done by humans.
Agreed, to a point. In many contexts, it is important for humans to do things themselves occasionally in order to learn how, even if a computer is generally used to do those things far more quickly.
Agreed, computers are generally way better than humans at catching many style issues. We utilize linters extensively, those enforce many of our style guide rules. However, there are some, such as the example pictured in the article, that aren’t always detectable with a linter. In these cases, we use the style guide as the source of truth to reduce the amount of personal preference that comes into code reviews.
I agree with that if you would be learning a language. But even then, being bothered by indenting, alignment, spacing etc gets tiresome and the small amounts of time lost accumulate over the day.
Depends on the kind of style 'subject' I think. If it is trivial indentation, bracket placement and other preference kind of things it probably wouldn't. But if the style things lean more towards design patterns, then often there is a proper reason behind choosing that style over another and it would help because you would notice doing it the 'wrong' way and you stop to evaluate and maybe spot a bug or design issue. I think that would fall in the category of code smells like when you use a framework in a convoluted way you're probably not using the framework right or over stretching it.
I'd second this. Depending on the style in question, there could be a mismatch in thinking, but probably not for most styles. If you're used to writing very dense C code for example, transforming it at the end to meet typical Enterprise Java standards might introduce subtle logic issues (or point out existing ones) or make it harder for you to understand in the future depending on how the transformation is done, but this is really just another instance of the general refactor caution to lean on tools, incremental changes that preserve key features like function signatures, and tests to make certain you don't change behavior.
Styles like forbidding single-statement if conditions that don't have curly braces helps protect you from bugs like goto-fail[0] meanwhile styles like which line your opening curly goes on, use of spacing, dare I say tabs vs spaces (outside of languages with significant whitespace anyway), and even abbrev names vs BigLongNames[1] don't really matter apart from personal preference and it being aesthetically pleasing to have local consistency.
This is a very uncharitable interpretation of the parent comment.
The parent comment appears to be referring to having people keep the shared style guide in mind when writing code, vs just letting the linter fix it. Extrapolating this to suggesting they don't want a team "with a variety of backgrounds and approaches" drags the conversation pretty far afield of that question.
The trick is to ban even the correct but (to a human) ambiguous patterns. In this case, I wouldn't let you do assignment on the same line as the conditional.
With an `if` it's probably superfluous and therefore bannable without cost, but sometimes it can make a `while` loop so much more concise - since the alternative would be to write the same code before the loop and at the bottom of the loop. It is the cost of inexpressive languages.
In many people’s head-parser, the first one looks like an equality check (but is in fact assignment). The parens try to clue people into “this is an assignment expression!” I think, but personally I wouldn’t allow an assignment expression in an if statement even with parens.
While you have a point, I don't think the exception should disprove the rule. Even if a few specific style patterns can't be automated, the bulk can be and it's probably better to have them enforced at the pre-compile stage.
Based on my experience with Mozilla's mature code review culture, this article is excellent. If I were going to make one suggestion, it would be to emphasize the points that author jelambs makes below: this approach to code review is inseparable from a commitment to deep shared understanding of the code base. It both depends on and supports that understanding. It's an investment in quality as embodied in a particular technical approach and body of work. It's not cheap, but what it buys you can be very valuable.
Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is.
Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just in code reviews.
I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive. It's been a looooong time now, still searching.
We dont have that problem where I work so it does exist. I've sort of experienced something similar myself when I've been doing reviews though - the feeling that you've got to find something to comment on. I've also been "star-struck" by getting reviewed by someone very senior or "famous" in the open source/tech world and basically blindly doing what they suggest.
The problem we do have though is huge queues for qualified reviewers and getting randomly allocated a reviewer somewhere on the other side of the planet so that even if they have minor comments (e.g. trivial typos in comments) you're looking at a 48 hour turnaround to get your code checked in sometimes.
I guess for the age/hierarchy thing, it could be possible to make the reviews double-blind via the tooling? I.e. reviewers dont know who they are reviewing, and the reviewee doesn't know who is reviewing them. Obviously once the code is committed then everything would become clear and transparent. Might help in larger orgs, although I dont doubt that it would be fairly easy to make an educated guess who is writing the code a lot of the time, and you'd be able to dig into git/whatever to see the details, but just hiding it in the review tool might go a long way to prevent unconscious biases. If someone has a real axe to grind is deliberately going out of their way to find out who wrote the code during a double-blind review so they can make special comments then you probably have a bigger problem.
This is one failure mode that definitely exists. Another failure mode is less experienced folks who believe all their code is perfect and take criticism personally.
I honestly don't know which failure mode is more common. I do know that I had the latter problem in spades when I was earlier in my career and that I'm really paranoid now about being that senior person on a power trip.
But if everyone is trying really hard to both act in good faith themselves and assume good faith in their coworkers, careful reviews are the best way I'm aware of for getting great code into the tree.
> still have to find something to gripe about because I have 20 years on this kid
I wish I had someone with decades more experience than me taking the time to find areas where my code could improve. Regardless of their motivation, this sounds like an amazing resource that you're lucky to have.
I think their motivation still matters quite a bit. Just because someone has 20+ years of experience doesn't mean they're actually leveraging any of that for their review. If they're nitpicking something unimportant and they're only doing it for political reasons, then in that act they are contributing negative value to the team, regardless of experience.
If someone is that much more experienced than you are, it can be hard for you to tell the difference between nitpicking and something that experience has taught them is more important than you realize. Even if it is just nitpicking, that still means your code has nits to pick.
I don't believe this. Every time I've seen subtle problems that experience taught me were more important than first appeared, I've been able to explain why. And so I put that explanation in the review.
Usually people complain to me about nitpicking for small things I thought should have been obvious and didn't require explanation. When you point out a word in a comment is misspelled, it shouldn't require an explanation about why proper spelling is important for readability and clear communication.
Code reviews are generally public and visible to the entire team and often even to other teams. While it's true that the reviewee may not be able to distinguish the two cases being considered, there may be other developers on the team or in the company who witness the review and can make the distinction. In fact, this can cause the "toxic reviewing" practice to spread to other experienced senior developers, who may see others with similar positions doing the same thing and then feel they must do it too in order to signal that they are, in fact, senior.
Interesting - I thought you would end the argument differently. In my experience additional visibility helps curb the unwanted behaviour because other senior devs step in and help fight it - but I guess that depends on the general culture in the company. If that doesn't happen you have bigger problems anyway.
I do believe that code review culture ends up being a reflection of general company culture. In a healthy culture, visibility tends to spread the positive part of what's observable, whereas in a toxic culture visibility tends to spread the negative and political parts of what's observable.
And there's definitely a circularity here. In a healthy culture, code reviews tend not to be overly political in the first place, so it would stand out like a sore thumb if someone was causing a lot of unnecessary drama with their reviews. Likewise, in a toxic culture, code reviews may go from healthy to toxic and dramatic because developers have been incentivized to compete with one another in counterproductive ways.
As an engineer, how do you address this sort of mutually-reinforcing jerkery? I expect that complaining to the manager will not be effective because the culprits are not explicitly hostile.
Easiest way? Complain, look for another job, quit. There is no way you can change the culture from bottom up. Best you can achieve is make a bubble with your close coworkers where you cover each other - but even that is temporary and can be broken by management anytime. Sorry. But there are many companies with better culture out there...
I've never been in a work environment where senior developers are so insecure about their seniority. How can a junior developer tell the difference between this scenario and one where senior developers are making good comments on code reviews that junior developers don't see the value of?
Unless code review is a rubber-stamping process (in which case why bother? just party on production...), you're inviting criticism and improvement, so freaking out about receiving some and getting defensive or calling people toxic is... less than productive.
One prolly shouldn't be a jackass as a reviewer, but if comments or questions on what you're doing get your back up, one should perhaps grow a thicker skin as a reviewee.
I think the word "gripe" was meant to imply criticism that isn't constructive and has no actual value in improving the end product.
Let's say you send code out for review that is just about perfect how it is. The reviewer is faced with a choice. If they don't suggest any changes, they may feel it gives the impression that you are just good a coder as they are, that their input isn't needed, or that they didn't bother to read it and just rubber stamped it. All of these challenge their place in the organization as a technical thought leader and influencer.
Some people have the maturity and awareness to just say, "Great job. I see no reason to change any of this. Approved." Some people don't, so they will dig for something negative to say. Even if it is just "these variable names are excessively long, and it's distracting to read" or "you don't have enough parameters to warrant the use of the Builder pattern here, so take it out".
Author here, I think that’s a great approach. We surface our guidelines for every PR by including a link to them in the description template. This acts as a reminder for people to be thoughtful about their review. I think the fact that our guidelines were based on input from the most senior to the most junior engineers says a lot about how much we value everyone’s opinions and perspective, not just the loudest voices.
Maybe because many people haven't experienced this? The parent poster also said this:
> I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive.
I didn't downvote the comment, but maybe some people think it lacks perspective and concocts theories for why criticism of their code is actually the fault of reviewers rather than taking it as a learning experience. But I didn't downvote it because who knows, maybe the commenter has actually had a bunch of bad faith reviewers.
We recently instituted a code review process at my company. We're small, 4 engineers on the core system, plus our CTO. All 5 of us review every commit now. Personally, I take issue with any bug that makes it into production, especially if its mine. We inheritted a code base that was written by cheap off shore contractors and it shows. We're working on cleaning it up and improving it, but it's slow going with a small team and tons of new requirements. I personally just committed changes to +600 files...we're still reviewing after a week.
> I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive. It's been a looooong time now, still searching.
They definitely exist (I work at such a place) so I wish you the best of luck in your search!
- coding style violations MUST be fixed (ideally all of these are caught by lint but not all are)
- everything else is treated as optional
Some code review tools even provide the ability to specify required vs. optional feedback. We now use github reviews which doesn't have this feature so we usually just add OPTIONAL to any review comment that isn't in the MUST fix category.
Occasionally a reviewer and reviewee will get into discussion/disagreement over the approach a particular code change. In those cases if it isn't clearly a bug or a style violation we err on the side of the reviewee.
Author here, that’s a great question! We approach this by ensuring that there are always multiple people that can review a given codebase or service. In practice what this looks like is we have teams that own our different services. Rather than just assign a random person from that team if you’re making a PR for one of their services, we utilize what we call “open” slack channels, where any one can ask questions of the team. So you’ll go to the teams’s slack and ask for someone to review. We recommend @ mentioning the people who’ve most recently made significant changes to the code you’re working in as they’re most likely to be the best reviewers. But by sharing the responsibility across each team, that ensures there’s no piece of code that only one person across the whole engineering organization is qualified to review. For projects within a team, there will generally be one or two dedicated reviewers and we utilize stand ups to check in and make sure no one is overloaded with reviews. And since the whole team has familiarity with the project, it’s easy to swap out a reviewer if someone is overwhelmed or on vacation.
Not everyone is the right person for a specific code review. Even on small teams someone may be more of a subject matter expert in one facet of your system or code. Therefore load balancing isn’t really the right way to look at it. The idea should be to seek feedback inclusively and welcomed from all teammates. As a submitter your job is to make sure you get the right feedback if there is someone more knowledgeable of a topic. This is why standup is a good place to discuss a code review where you can identify need for time and also identify right people.
Agreed it shouldn’t be an even distribution of review requests across the board. That said, if a few people are getting all the reviews, it’s difficult for them to make progress on their project work, and for other engineers to learn code review best practices and get familiar with the codebase.
It’s a balance but you need to balance team priorities. If you aren’t shipping code because of your time on code reviews you need to better manage your time. Call it out during stand up (assuming your team has them) and make it clear you don’t have time or code reviews today because you need to focus on your deliverable. If the review is more important your team can identify this collectively. However, individuals should never feel you describe, if so it’s a lack of coordination across team.
I always thought that it was ideal that the most experienced person does the bulk of code reviews and clicks the merge button even if that means that they don't write much code themselves. It's a model that, for example, works very for Linux.
I think that there's a lot of value in having somebody who has a picture in their head of the entirety of a system who can spot impedance mismatches across projects and spot germinating code smells before they cause major problems.
How common is it to get unsolicited reviews on your PRs? In my team, I have some colleagues who will routinely pipe up when they see a PR being uploaded by an "outsider" and will write a litany of complaints in order to block progress and establish their dominance. (I should mention that these colleagues work in a branch office, so this is their only way to assert themselves.)
This is your subjective interpretation of the facts. Learn how they complain (in the end, they only can complain about technical facts: learn them, and be correct next time), and you will not receive complaints anymore.
The real problem here are people with bad attitude and poor communication skills. Bullying other people is a punishable offense in real life, and I don't see why code reviews should be any different. Have some basic rules regarding communication. Issue a public warning for first-time offenders. Give them the boot if they do it again. There, problem solved.
It seems overly simplistic to say that effective code reviews are inevitable if only you can prevent people from being rude. Certainly not a bad START, but I dare to think there may be additional ways a review can be a GOOD one as opposed to just not being a BAD one.
It's easy to forget that we're still learning how to write "good" code, and that many of our best practices continue to evolve, occasionally into VERY different things. A Code Review has to be able to identify poor practices as distinct from UNUSUAL practices, with that latter evaluated in terms of cost-benefits and not forgotten even when the costs are too high in the current project - that may be a practice to try out where there's less impact to being inconsistent. A Code Review has to balance the subjective opinions of multiple devs to try and achieve some consensus that is treated as objective. None of this is easy nor truly figured out, and not being a jerk is just one of multiple steps to accomplish.
You sound like you didn't read the article. There's nothing about political correctness or try-hard inclusivity in that article but many (or indeed all) the things that common sense would dictate as a base of working code reviews.
@tdsamardzhiev What sorts of "bullying" have you seen in code reviews before? Having some anti-pattern examples could be useful to codify into a developer guid.
Neat to see that they started by interviewing a diverse subset of their engineers; that type of user-centric research is invaluable. Moreover, I wonder what of their methodology could be mapped to a live analysis of the code review conversation threads they're having via GitHub.
More than anything else, though, I'm curious where teams like this still go to buy those amazing Apple Cinema displays?!
Not focusing on style violations is foolish. Imagine someone submitted an English paper littered with grammatical errors and typos. It would be difficult to find the intended meaning and bigger picture. Style violations are the same, they have to be fixed before a deeper review can even take place.
Everything a computer can do trivially should not be done by humans. There are plenty of tools available to validate code to all the standards you can dream up. And most languages allow you to even automatically fix style errors or even enforce it for compilation (thank you Go).
So instead of codifying it in a style guide document, codify it in config files for linting and autoformat tools instead. The defaults of those tools are often fine for what you need and exceptions can as easily be documented in them as well.