Hacker News new | past | comments | ask | show | jobs | submit login
We are ruthless on code reviews (workiva.com)
51 points by grey on Apr 27, 2016 | hide | past | web | favorite | 69 comments



- If this post is satire, it's hilarious and spot-on!

- 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.


It's not satire, it's hypothetical.


It reeks of humble bragging and pretentiousness. The Dark Souls analogy just tops it off...


Reading it again, I can see that. However, I personally am a fan of rigorous - not ruthless - code review, the purpose and level of which has previously been explained to a new employee.

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.


Decent positions dont usually need disclaimers. Theres a difference between preaching, teaching, and coaching. Its not obvious that being an asshole is out of line. You make it sound like this mystery of pure "collaboration" is all about giving raw feedback. Raw feedback is inferior to polished feedback. Its just as important to communicate in positive ways instead of throwing this ruthless word around.

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.


Haha ok dude. I didn't write the article. And that wasn't a disclaimer; I was empathising with your point.


Oh, well carry on then, my bad ;-)


If you have things in your style guide that can be done easily by a script, they shouldn't ever be interm rules. They should be a linter auto-fix rule that applies it to diffs automatically and NOTHING ELSE. It shouldn't even blip at you saying it's against style, it should just fix it.

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.


I love working in environments like this. I can say hands down they were my most productive years as a programmer.

Quality code review is the single most important trait of a high performing senior programmer.


Before people take and run with this comment, this will turn into a shit show if you don't have actual senior programmers with the needed experience. I've seen it happen in startups where their "senior programmer" has only been working for three years out of school for the single startup.


This is absolutely the truth, and based on my experiences, absolutely a problem. New graduates are promoted through the ranks at startups so quickly, yet they have terrible interpersonal skills, and their code reeks of naivety.

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.


While I understand where you are coming from, I don't see how your comment adds anything to the conversation.

If you don't have senior staff then you are going to have a host of other problems as well.


As someone who works at Workiva, I completely agree.

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.


Code review is typically how I structure my interviews.

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.


My first internship was like this. I loved it. It provided me with instant feedback on my code and as a result the quality of my code went from probably pretty low to as good as I could make it.

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.


People find it acceptable to write more harshly than they'd speak face-to-face. I wonder if all 43 or whatever points would have come up if it was done side-by-side the developer.

Also, great negative PR for this company. This sort of discussion would also come off as much less arrogant/snarky face-to-face.


>I don’t mean we’re mean-spirited. I just mean that we are merciless. 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.

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.)


Until the first time you spend a couple of hours looking for a weird bug in Python because somebody broke this rule...


Which rule?

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.


You commented on the import ordering so clearly that's what I'm commenting on.

Not following the PEP standard for python import ordering can cause issues like circular imports or monkey patching not working properly.


It always astonished me how delicate coders in the UK are (I am from Germany, working in the UK currently). During a team meeting at a former company, my "ruthlessness" was brought up as a negativism. I countered that (or so I thought) with the following question to the rest of the coder team: "Do you want me to point out problems I perceive with the code / approach / whatever, or do you want me to hold back in order to be considerate of someone". To my surprise, in the presence of management, the unanimous answer was to show consideration!

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.


There is another side to this. I've seen a ton of technical criticism get delivered with emotionally laden commentary. X is "totally broken", "crap", "not even wrong", "just wrong", "terrible", etc. There is a way to deliver criticism while maintaining an environment of common professional respect. Many people don't even try. With the caveat that I'm painting with a culturally biased brush, I've found Germans (and Finns!) to be particularly abrasive in this regards.


And conversely, I've seen people from cultures with different communications styles substantially and unfairly discriminated in the industry for not matching 2016 American business culture. I've found Germans (and Finns!) to be particularly discriminated in this regard, primarily because those are highly technical cultures, but with different communication styles. However, anyone who grew up in the inner city, in African American culture, in Irish Catholic culture, etc. is likewise stigmatized and labeled as rude due to what is a cultural communications difference.


I'm glad you wrote this because I would definitely never consider a person with such an obtuse attitude for any position

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)


I am all against pedantic nitpicking. One of my most common complaints was to concentrate on the actual problem, not on the style. I am not a "code craftsman", I am somebody who knows a lot about how to solve a problem with code. And I don't work for anyone except myself anymore.


It's not what you say it's the way that you say it.

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.


It is very difficult to do code reviews when beside the technical topics politics come into place. After 8h of reviews on 2000 lines of code finding 100 issues, it is almost not possible to be nice anymore. But hell starts when manager decide to give a GO because deadline approached without even fixing the 3 major bugs.


It's interesting that even your comment is getting down voted. I wonder if it's a millennial thing? It would be interesting to know the ages of the people here commenting on how "harsh" the post seems vs. "yeah that's great."

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?


Yes, it's extremely patronizing. A better email might be:

"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.


So they didn't ask you to point out the problems in a more considerate way?

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?


Programming tends to attract a lot of people who are smart but too young to have wisdom. The kind of wisdom that tells one when it's worthwhile to be ruthless and when it's just pedantry, self-defeating, etc.

The kind of wisdom that keeps one from positing that the people who don't see things their way are delicate.


Wisdom is overrated. Also, I am probably older than you are :-) I am using "I" and "You" because I like direct communication instead of passive/aggressive snobishness.


> ... because I like direct communication instead of passive/aggressive snobishness.

See, wisdom is not saying stuff like this.


I miss working in an environment like this. I was actually surprised to find so much negative feedback in the comments.

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 can tell you that this is the kind of environment I work in and the first thing I teach a new teammate is that the team is only concerned with consistent high quality code. I think it is a good lesson for most developers and software engineers to separate their personal feelings from their code. It can help you be less defensive of your approach and take in multiple viewpoints to find the best possible solution for the task at hand.


The whole "we are so cool because we are ruthless. Merciless, I tell you! Once you get through our code review your code will be perfect." attitude of this article is a bit surprising for one ranked so highly on HN. Do people really prefer this kind of attitude? Bender GIFs and snarky comments can become quickly tiring.

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.


I agree. I'm all for rigorous standards of code review, whether I agree with every convention in question or not. However, after the author mentions writing "beep!" instead of explaining an import order convention (as if it were challenging to copy/paste a single sentence a few times) and putting obnoxious gifs in their review, not to mention the self-congratulatory tone of the article, it doesn't make me feel like this is a place where I want to work. Encouraging best practices needn't come at a cost of being blithely pompous.


Every single time I do a code review I find myself noticing a lot of little style mistakes (and, not to be overconfident, I could be mistaken myself about that) and don't leave a comment because I don't want to be that guy that no one wants to review their code and gives one a major headache because I care more about the consistency than does the person I'm reviewing.

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 a team, decide how much you care. Some teams won't care much at all, and will follow a "hey as long as it works" attitude. Other teams will be deeply interested in making their code as good as it could be, and will want comments about doing things more cleanly or elegantly and will view PR review as a chance to learn and improve.

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.


Yeah I think this kind of thing is a teamwide preference. If you prefer very rigid styling and enforce a style checker, great – that can be put into a script which you compulsorily run before review.


For me declaration of "ruthlessness" is a red flag, similar to ninja buzzword.

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.


God, how insufferable. "Our code reviews are so ruthless because our clients (and by extension, us) are so much better than everyone else!" But apparently we can't write a bot or a pre-commit hook to re-order imports so instead we'll write some non-sensical in-house jargon (because typing out the full explanation is too much effort, even though we are the best of the best because our clients are so excellent and demanding). But don't worry, we'll write a 2 page email to explain this jargon.


I guess that's one way of doing it, but you're dependent on the quality of said ruthless reviewers, and delivery of the message is separate from the message itself. Personally, I'm more of a "kill them with kindness" guy, so while my reviews would come off as soft by the article's definition, I'd argue that they're just as effective, and my teammates still want to have a beer with me at the end of the day, instead of walking away thinking I have a stick up my ass, which an email about ruthlessness wouldn't necessarily negate.


If it's anything like where I work, some people come in for more ruthlessness than others. Some are more equal than others, if you will. New people or those deemed unworthy will receive the full brunt of scrutiny, to the point of bare-faced pedantry. Those who are golden are given the benefit of the doubt. You can probably tell which one I am. It gets really old to see this time and again, and to see the team pat themselves on the back for being "ruthless".


That's a failure of your team lead. Tough code reviews are important, but I don't tolerate people being petty or nasty. We actually train new employees on how we like to do reviews.

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.


Yeah, I know it is.


Playing devil's advocate here: The only way to really learn the full ruleset of a team is by undergoing a couple of code reviews in which every issue is pointed out. Once your teammates see you've understood the prevailing style, they don't have to work as hard at code reviews.

That said, I usually hate petty/nitpicky code reviews. I try to focus only on issues that I think affect readability, maintainability or functionality.


Makes sense, but there are ways of being even petty and nitpicky without being condescending or rude. State facts, don't throw insults, don't demean or belittle.


One good rule I was given by a tech lead is to not use the word "you" in the PR comments. PR comments are a discussion on the failure of the code, not of the individual. Avoiding "you" allows for some separation between the two.


I always use "we", and I almost always phrase things as a question.

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?"


The point is that two different people can post the same code and get starkly different reviews. Senior people get a thumbs-up emoji, others get tedium and pedantry. So you can tell that it's not really about the code.


This reminds me of people who say, "People don't like me because I'm so honest." No, people don't like you because you lack basic empathy.


Amen. Bonus points if they try to dress it up with some honorific like "Truthful Speaker".


s/lack basic empathy/are an asshole/


Code reviews are pretty heavy going on the project I'm working on at the moment. The code isn't noticeably better for it though!


You gotta argue your side if what they're saying is going to make shit awful. IMO the best way to do it is to try their approach out and show them in a few lines that you don't think it's helping. Doing whatever people say just because is bad and so is just saying no. You gotta work with them.


Don't worry - there's been plenty of long fruitless debates along the way.


Yet despite being assholes (ruthless in their parlance) who obviously have not on-boarded said employee or even told him what is expected of his code, I doubt their software has any less bugs than the software from shops who are not assholes (ruthless in their parlance). Sounds like a great reason to avoid working here and other places that have the same mentality. If you want your coworkers to actually do their work, it's a really good idea not to be an asshole because it's incredibly easy to get away with not doing shit in tech while seeming to do so and after awhile, most people will switch to that mode when berated long enough. I can't blame them. Employers deserve no loyalty in the US these days because they don't give any.


In this articles terms, I'm ruthless when doing code reviews. I do it because

* 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.


'We are rigorous on code reviews' would be a better approach. We don't have to be jerks to make sure that code is up to standards.


"One thing to keep in mind is that our customer base is unique. Our customers work for many of the most successful companies in their respective industries. They are where they are because they are committed to excellence and have great attention to detail."

An awful lot of companies can describe their customers in such terms. Microsoft can, Google can, Pepsi and Bic can.


Satire, yes?

If not, GTFO while you can.


As a PHP "shop" this is why we use StyleCI [1].

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).

[1] https://styleci.io


Based on this I can only assume that workiva builds spaceships or life support systems. You know, something where absolute perfection is far more important than having a fulfilling and constructive workplace environment.


It's probably a joke but it's backfiring because most comments are taking it seriously and thinking the place actually works like this.


Ugh, comes off as condescending, immature, and insufferable.

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.


If you've got time to dream up ways to be witty in order to be snarky to a co-worker, then you've got too much time on your hands at work and clearly need to fill it with other responsibilities.


I would hazard a guess that this letter was to a hypothetical employee, rather than for a real situation. And its purpose was to illustrate the type of code review new employees can expect and are expected to give, in a whimsical way. I quite liked it.


That's right! Get back to work. That's all you're good for, after all. Just keep working! Have a funny thought you want to jot down? Sorry pal forget it you clearly should be working, endlessly, tirelessly. Don't forget you're here forever. Working.




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | Legal | Apply to YC | Contact

Search: