Hacker News Reader

Comment by wrestlerman | Hacker News Reader

A lot of things, my experience is based on working in startups: 1. Suggesting silly changes in CR - instead of A you could have used B. So what? I used A, let's keep it that way, time is money. 2. Long discussions in CR 3. Too much thinking like a programmer. I'm not coding, because I love it(although I like it), I'm coding, because business needs it... 4. Asking for estimates, when we both know that estimates in this situation are impossible to predict. 5. Shitty written tickets. I don't know the app well enough yet, so one sentence tickets won't help me much. 6. Too much obsession over some technology. Everything has some good and bad sides, so stop selling it, especially when no one is paying you to do that. 7. Also, I dislike when you can't get a simple answer to your question, but you get funny looks that you don't know the answer yet.

View on HN

Oh yes, so often the CR process becomes a hassle. Somebody positions themselves at the door as some sort of guard, and harasses everybody going through. So often just to feel like they're the boss or something.

My son just quit a startup because that had become epidemic. Couldn't get emergency code committed; couldn't meet (artificial) deadlines for demos and shows; every choice yammered over for esoteric reasons.

I suggest to everyone: when you make a comment in CR, also approve the commit. Trust the engineer on the other end to do what is necessary and responsible. If that become a problem, address it then. It seems like the minimum in professional respect.

People's live are more dependent on software, even a simple website can leak info that has devastating effect on folks. Treating software development casually is a big problem. If we are going to be professionals, then we need to be professionals, most folks love to call themselves "software engineers", then let's act like one. Would you like if the same approach was taken on the car you drive, elevators, planes, medical equipments, buildings? Building great software is a science not an art. If you wish to express yourself, go code demos, design levels and game play.

Let me say this because I don't see it often said. Coding guidelines & Code review are a balancing act. One without the other is useless. Code review is where the correctness of the code is verified and we make sure that the guideline is followed. Without a guideline, the review process is just opinions. A guideline but without peer review is just a waste of time too. Guidelines must be reviewed often, it's a living document, that must be continually revised if need be to match the team and development process.

If your guideline calls for indentation by 2spaces and you use 4, it's not harassment, it's a violation of understood principles.

Now I must agree that the CR process should not be a bottleneck, if it is, it's a symptom of a bigger problem with the team and leadership. It's the leaders job to identify whatever it is and clear it so it can be fast and efficient.

I don't understand why people hate CR.

You're a professional you should be able to defend your code, and explain your reasoning. If you can't, you need to change it, and you'll improve at the same time.

I mean most changes in code review only take 5 minutes to rectify, so I often don't buy the time argument.

If you getting bit architectural change problems in your code review, you obviously didn't collaborate with the team very well... People should roughly know what your approach is before the CR even arrives at their door

I do think that code changes take more than 5 minutes, why? As a person, who pushed the code: 1. I have to read the comments 2. Decide if they are right/it's worth a hassle 3. Sometimes you have to reach out to the author of the comment to clarify some things. 4. Change your branch 5. Change your (thinking) context 6. Apply stuff 7. Push stuff 8. Notify the person that it's done. Many times it's more than 5mins, more like 10/15mins+

I don't hate CR. Someone below explained it better than me, that different people see the code differently. And you should respect that someone wrote it that way and not try to force your way upon them.

> If you getting bit architectural change problems in your code review, you obviously didn't collaborate with the team very well... People should roughly know what your approach is before the CR even arrives at their door

I agree with it a lot. I think that would solve a lot of problems, but not necessarily the small ones.

Yes, but that's not what happens. Folks say "you tested for an empty pointer wrong" meaning not how they prefer. Or "This could be coded another way that I think is clearer".

Here's a litmus test: if the comment has nothing to do with the correctness of the solution to the ticket, then don't comment. Instead write another ticket to address {whatever}. If it doesn't rise to the level of a ticket then guess what - you're being pedantic.

>Somebody positions themselves at the door as some sort of guard, and harasses everybody going through. So often just to feel like they're the boss or something.

I've noticed this outside of the CR process as well. When people are tasked with providing a critique, it's as if they feel obligated to say something because they're treating the task as "find something wrong with X" not "determine if anything is wrong with X". Sometimes it seems like people trying to exert their power, but often it's more like people are just trying to prove that they're adding value.

> Sometimes it seems like people trying to exert their power, but often it's more like people are just trying to prove that they're adding value.

If you work with people like this a lot, try introducing a shit sandwich to your company. Before you call your caterer, let me explain what that is...:)

My Dad taught me that when you have to criticize/correct something, do so in a shit sandwich. Start off with positives, then express your criticism and then end with positives. Thinking about positives tends to ground people and get them to evaluate the situation, not merely come out with negativity to prove their worth...

This is common enough that everyone would know what you're doing and think you only said the nice things so you could say the negative :P

Good point - sincerity is a big if in this process. If the good is genuine, this is an acceptable tradeoff, especially because thinking about positive things often makes the negative more gentle and more constructive.

> when you make a comment in CR, also approve the commit. Trust the engineer on the other end to do what is necessary and responsible. If that become a problem, address it then. It seems like the minimum in professional respect.

I agree wholeheartedly with this. One of my biggest pet peeves right now is having PRs delayed from merging because I can't get the required approvals over tiny non-vital changes.

IMO a code review should only ever be rejected / not approved if it a) will bring down prod upon being committed, b) contains easily correctable but serious oversights (bad run time algos, potential NPEs, etc), or c) is built in a way that is not easily transferable / maintainable / goes against establish patterns by the team.

Too many people use CR as a way to gatekeep and show off. I'm all for discussion, possible improvements, exchanging opinions, and learning - but not at the cost of leaving PRs in CR status for days on end.

I can't personally agree with that viewpoint. If I know you're going to introduce a problem, it's my responsibility to say so then and there. If they're professional they need to respect that. Especially if it's CR from a junior.

It's not a viewpoint; its a process. Comment and approve. Don't comment if its unrelated to the ticket being address; then write another ticket if the issue warrants it. Otherwise, save it for around the water cooler.

> I suggest to everyone: when you make a comment in CR, also approve the commit. Trust the engineer on the other end to do what is necessary and responsible. If that become a problem, address it then. It seems like the minimum in professional respect.

When we do code reviews, we try to make things explicit. What needs to be changed, and what am I just commenting on to illustrate a different way to do it.

Sometimes code is just wrong (either at the micro level--you forgot to remove that debug line--or at the macro level--you forgot this edge case) and a second set of eyes is a great way to catch it.

But I agree, treating the original developer as a professional is a good idea.

A "why aren't you doing it this other way?" comment is appropriate when:

- You understand both options.

- You understand the tradeoffs between them.

- You have a reasoned argument for favoring one approach.

- You suspect that the other person is missing some of this information.

I love getting comments that teach me about a language, API, design patterns, or potential bug, and make my code better. Even if I'm going to stay the course in this particular case, I'm happy to have received the comment, and always try to let the commenter know that. An okay code review gets my code landed fast, a good code review is one I learn from.

But I can't fucking stand defending and explaining perfectly normal and reasonable choices to engineers who just don't know and are curious, or have weird taste, or are otherwise putting me through the exhausting exercise of fighting for every little choice without providing value in return.

> But I can't fucking stand defending and explaining perfectly normal and reasonable choices to engineers who just don't know and are curious,

Often times when I'm reviewing code, I'm learning as well. If I ask a question because I don't know and am curious, I'd think that's a good opportunity for knowledge transfer. Why can't you fucking stand it?

edit: it occurs to me that this comment is extremely meta, hah.

Code quality is an aesthetic sense. It's easy to look at some code and render an opinion. With a certain level of maturity, you can intuitively come up with code that feels right. But offering a rational explanation for each minor decision between mostly-equivalent alternatives is disproportionately hard. Usually there isn't one. Trying to get more experienced engineers to knowledge-transfer the decision trees they're following is a seductive but wrong model for getting better at software engineering. If such trees even exist they are subconscious.

Observe, reflect, try stuff out, solicit feedback. Run that cycle with the highest quality and quantity inputs you can find. It's absolutely your senior colleagues' duty to help you develop the aesthetic sense, but the higher quality and more cost effective way to do that is by giving feedback on your work.

Whilst I agree with a lot of the points you made I'm not sure on #3 - I don't see that as a negative when compared with the opposite ( devs who don't care or 'think like a programmer' as much as they should ).

Similarly on point 7 I get very excited about new tech ( or similarly with cool old stuff i've only just discovered ). Who needs to be paid to be passionate about what benefits new tools may bring for me or the team :)

I don't think it's wrong to get excited about new tech. But from what I have observed it's mostly that one dev that pushes for new stuff and after they get sick/leave someone else has to take care of it and many times it means a rewrite of that functionality. Also many times with new tech, you can't really tell its bad sides, because it takes some time to know if it's solid or not.

This list kind of makes me want to work with you! And I didn't even work in startups... I guess that experience can be very widespread...

Agree with most of your points. For 1 and 2 it depends on what constitutes silly and the situation. I've seen some terrible code go through CR in the name of speed. For loops in for loops in if statements in for loops with breaks and continues abound. It worked, but be damned if I wanted to see it again.

What is CR?

Thanks for #3. Writing code is my profession. I don't love it, in the same way that a plumber doesn't love unclogging your toilet. He has to do it because he needs to buy food.

Reply on news.ycombinator.com