We are ruthless on code reviews • Hacker News Board

Re: We are ruthless on code reviews

#51

Post by kvcrawford » Wed, Apr 27, 2016, 4:35 PM UTC

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.

Re: We are ruthless on code reviews

#52

Post by cafard » Wed, Apr 27, 2016, 4:41 PM UTC

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

Re: We are ruthless on code reviews

#53

Post by Dommorak » Wed, Apr 27, 2016, 4:43 PM UTC

kvcrawford wrote: post #51

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…

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.

Re: We are ruthless on code reviews

#54

Post by falcolas » Wed, Apr 27, 2016, 4:45 PM UTC

bobbytherobot wrote: post #35

gjolund wrote: post #2

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.

Re: We are ruthless on code reviews

#55

Post by auggierose » Wed, Apr 27, 2016, 4:57 PM UTC

gdulli wrote: post #46

auggierose wrote: post #28

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…

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.

Re: We are ruthless on code reviews

#56

Post by goldenkey » Wed, Apr 27, 2016, 5:18 PM UTC

cpjk wrote: post #49

goldenkey wrote: post #47

Earlier quoted context omitted.

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.

Re: We are ruthless on code reviews

#57

Post by crdoconnor » Wed, Apr 27, 2016, 5:23 PM UTC

Ensorceled wrote: post #50

crdoconnor wrote: post #29

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

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.

Re: We are ruthless on code reviews

#58

Post by draw_down » Wed, Apr 27, 2016, 5:25 PM UTC

noamsml wrote: post #5

draw_down wrote: post #3

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

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

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.

Re: We are ruthless on code reviews

#59

Post by draw_down » Wed, Apr 27, 2016, 5:25 PM UTC

cachemiss wrote: post #4

draw_down wrote: post #3

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

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" the…

Yeah, I know it is.

Re: We are ruthless on code reviews

#60

Post by nmeofthestate » Wed, Apr 27, 2016, 5:34 PM UTC

ybobinator wrote: post #39

nmeofthestate wrote: post #22

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.