Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

We are ruthless on code reviews

techblog.workiva.com

Post reply on HN
Print view View original

21–30 of 72 posts

Re: We are ruthless on code reviews

#21

Post by thinkpad20 »

parennoob wrote: post #9

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 conser…

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.

Re: We are ruthless on code reviews

#24

Post by mahyarm »

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.

Re: We are ruthless on code reviews

#27

Post by aelaguiz »

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.

Re: We are ruthless on code reviews

#28

Post by auggierose »

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.

Re: We are ruthless on code reviews

#29

Post by crdoconnor »

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

Re: We are ruthless on code reviews

#30

Post by cachemiss »

coredog64 wrote: post #20
Niksko wrote: post #8

Earlier quoted context omitted.

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

Post reply on HN