We are ruthless on code reviews
techblog.workiva.com
techblog.workiva.com
- If this is real, quit while you can, and may god have mercy on your soul.
- The fact that it's difficult to tell if it's real or not says a lot about our industry.
Obviously being an asshole is out of line, and if someone is being a dick in code review, that's a problem with the reviewer. But I think that being rigorous about maintaining consistency and quality is important.
Honestly, your problem is that youve approached code discussion from a purely quantitative approach - ability to convince and ability to convey arguments in a persuasive yet unbiased way is huge. No one wants to do things by demand, decree, or just general hate.
If you don't have this, you waste everyone's brain cycles with your pedantic import ordering rule or similar on both sides and it's a red flag about the company to me.
Quality code review is the single most important trait of a high performing senior programmer.
I've watched an "architect" from such a company write the most tangled spaghetti western of code for the simplest CRUD app that even his lower level colleagues were appalled. Of course, those same colleagues then advocated a complete re-write of the working codebase instead of incrementally fixing the code, causing about 6 months of new work for the company before they could start on hardening or new features...
No, experience doesn't automatically make you right, but it does increase the likelihood.
If you don't have senior staff then you are going to have a host of other problems as well.
The post comes off a little more abrasive than I'd like. I think writing it as an "email" to a hypothetical new team member makes it worse.
The point is: we take our code reviews seriously and we may point out things that seem silly or nitpicky, we may question your approach, we may make many suggestions for improvement, but at the end of the day it is nothing personal. It is for the common goal of high code quality.
I try not to ask arbitrary comp-sci questions, but instead provide the candidate with a block of code and ask them to provide a comprehensive code review (usually through a github pr).
Afterwards I perform a my review, and highlight any differences in style or methodologies. I'm not really looking for people that have the exact same outlook as me, I'm just looking for people that have some kind of standard they adhere to.
Having now worked at other places where code review is given low priority, I can say with confidence that I don't want to work anywhere that does not do serious code review.
Code review provides an important mentorship opportunity for junior programmers, and provides a simple way to keep code quality high and consistent. In addition, it provides a simple way to instruct new hires on internal style and conventions.
Also, great negative PR for this company. This sort of discussion would also come off as much less arrogant/snarky face-to-face.
Translation: "We have extremely strong opinions on what materials to use for our nuclear power plant's bike shed."
https://en.wikipedia.org/wiki/Law_of_triviality
There's seems to be an inverse relationship between how much effort a team spends on surface details of code (e.g. does it adhere to linting standards/are the lines < 80 characters/imports ordered?) and how much time and effort a team spends on deeper architectural issues (loose coupling, de-duplication, un-reinventing the wheel, transforming imperative to declarative code, etc.)
The rule that imports must be ordered according to the whims of the team lead? Or the rule that lines must be under 80 characters?
There are a few linting rules which it pays major dividends to pay attention to (e.g. variables that are initialized but never used), but most of them are superficial.
Not following the PEP standard for python import ordering can cause issues like circular imports or monkey patching not working properly.
I think people who work together should be able to openly communicate with each other about technical issues. Sadly, most people are (either self-perceived or for real) not good enough to do that without bruised egos.
It's not about sensitivity it's about not being a inconsiderate pedant
I'm also sure that a lot of your critique has no basis in technical quality rather than personal preferences. This is what I see time and time again with the people that "worry most" about the code (but when it's their code that's critiqued it's full of problems). Uncle Bob fans, "code craftsmen" all those people make my BS detector beep
There are also people that pick a lot of "problems" in the code that ignore glaring issues in other areas/code they wrote/other aspects of the issue, including further code maintainability, readability (no, being readable to you does not mean it's readable by somebody else), etc
It's not so much about bruised egos rather than wasting company time with pedantic nitpicking, not trusting colleagues (which is fundamental and you seem to be utterly incapable of), overstating your own importance in the team (unless you really picked a company full of bozos, which in the end is your mistake again)
If you don't have the social acumen to know how to raise issues and to discuss them reasonably (i.e. without rubbing people up the wrong way), then yes, erring on the side of being considerate is likely to win you more friends and help you keep your job longer.
There's a fine line between constructive feedback and being toxic to a team.
I read the post as going out of it's way to give praise while pointing out what the company requires (perhaps that is interpreted as patronizing). But many people here are commenting on how rude it is. Bizarre. If you think this is rude, you've never worked in a "tough" environment.
How would people here suggest communicating the same information? Is it just that it might be interpreted as patronizing that's the problem?
"Hey, sorry if that code review seemed a little nitpicky. You did a great job, but we try to be as strict and consistent as possible in our reviews, for everyone's benefit. Here's a link to our style guide, and please let me know if you have any questions. I'll stop by your desk in a little bit to introduce myself and see how you're doing."
You don't need any exclamation points, and you don't need to talk about how unique and amazing your company is. Treat your colleagues like colleagues, not insecure undergrads.
Instead, they just asked you to stop pointing out the problems?
Were these problems nitpicks, personal preference (as some here are apparently assuming) or were they objectively technical issues?
The kind of wisdom that keeps one from positing that the people who don't see things their way are delicate.
See, wisdom is not saying stuff like this.
I think a detail getting glossed over is that the tone needs to be calibrated to the relationships between team members. If team members joke around with each other, send silly gifs, get lunch or coffee together, and are generally on good terms with one another...then it's known that you don't take PR comments personally, they're just feedback. And the more critical the feedback, the better developer you become.
I personally would strongly prefer if they put it in milder technical terms, "We are very conservative with deviations from our preferred code style and conventions". I suppose that wouldn't be as click-baity, but for me (and I suspect a lot of other people) it would be easier to appreciate, since we understand that being conservative before accepting commits leads to a well-groomed codebase.
In other words, I might restrain myself from criticism out of my compassion for my coworkers. The definition of "ruthless" according to Google is "having or showing no pity or compassion for others," so it's a fine term to describe how I'm not-- and how this company is determined to be.
As long as everyone's on the same page, and sharing the same attitude, that drive to improve will come across as a positive thing rather than a jerk move. But if you're the one person with that attitude on a team of good-enoughers, yeah, not so much.
Strictness in one direction does not mean company is strict at all fronts. Code reviews can be used without automated tests or version control.
In this case the review process seems like a "busy work". Checks bellow could be easily integrated into build script or CI. Such code should not even made it into code review. (I am not arguing about the rule itself, just how it should be enforced)
> You’ll notice that I left the comment “Beep!” on the imports of every file you touched. What I meant was, “Your imports violate our standard convention—we order them by built-ins, then third party, and then project level,” but that was too much to type on every file.
If I see people being given the benefit of the doubt, I call out other senior engineers. If I see people ganging up on a new person, I do the same.
A quote I've always heard, is generally when someone talks about how "brutally honest" they are, they enjoy the brutality more than the honesty. I filter for the latter, and come down hard on the former.
That said, I usually hate petty/nitpicky code reviews. I try to focus only on issues that I think affect readability, maintainability or functionality.
Example: "So we are doing X here, which I think will probably do Y, which could have adverse affects Z, are we sure we want to do this?"
* We have a code style to follow, and if we don't we might as well not have it.
* It ups the game from the developers, they double check their code before checkin, and believe it or not, they catch more errors this way.
* It have created an environment where we can openly talk about interative improvements.
An awful lot of companies can describe their customers in such terms. Microsoft can, Google can, Pepsi and Bic can.
If not, GTFO while you can.
Our code isn't that critical, the quality and maintainability are. We don't waste time on "code reviews" that only check the style of code, that can be automated away.
Yes, I make StyleCI, but we use it in the place I work too (unrelated).
I do think strict code review and high standards are the way to go, but when implementing them it becomes all the more important to have appropriate and professional interpersonal communication and documented policies.