Hacker News Reader

Comment by osivertsson | Hacker News Reader

My suggestion is try and get feedback earlier.

Whiteboard with your coworkers and architect what you are building once you have a pretty good grasp on how you want to solve a task. You probably have some code at this stage to feel pretty confident that it will work.

Agree on naming of concepts and design at this point, allowing you to change direction without reworking too much. Of course further changes to design will occur as you learn more during implementation. If they are major run them by your team again to keep them in the loop.

It is probably true that you could work faster, at least for a while, if reviews were more slack. That the company chooses to prioritize less risk instead.

Maybe it is just time to move to the next job since your learning curve is flattening?

View on HN

Indeed, it might be time to move on. Or at least talk to the reviewer that bothers me.

Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough.

I'm usually okay with all of these suggestions. Still, lately, I'm fell in a position where usually the person who reviews my code is our architect (an amazingly talented engineer). He always flags all the minutiae (very politely, of course), and I find it very hard not to comply as I want to ship better stuff.

Funnily enough, a few days ago, I found myself reviewing a colleague's PR and repeating the same behavior. As soon as I saw myself doing it, I deleted these comments, made the suggestions that I judged valuable, and approved.

Thanks for the input! Just replying gave me a lot of clarity on the subject.

Sounds like the reviewer realized he enjoys exercising power over others and feeling respected, and that metastatized into pedantry.

> most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough.

Jeez

> Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough.

This pedantry is a cancer. If your PR comment starts with "Nit:" just shut up and go do something useful.

I don't think so.

Nit comments are how I indicate areas where I would have done something different but I don't feel strongly enough to block your PR over it. Could be some formatting that I think could increase readability or some code you didn't touch in your PR but if you have time while you are in this file you could improve while you are there.

I always stress these don't block PRs and for the most part they are subjective. I still find it valuable to share these perspectives as they offer gentle ways to impart the smaller parts of "good taste" that make programmers well versed in such things a pleasure to work with.

"Good taste" is just your opinion. Framing it this way makes it sound canonical, which it is not. We all have opinions on what good taste is, and they all differ. To see this, all you have to do is go read the codebases of all your dependencies. They all taste different. Which is my point. These comments are useless noise.

> This pedantry is a cancer. If your PR comment starts with "Nit:"

I have worked in companies where sloppyness was culturally acceptable. I found that a much worse cancer because there is no fixed line when sloppyness is cosmetic and when it affects code quality and maintainabilty. Code quality just did deteriorate over time as little things started to accumulate everywhere.

Yes, review discussions are tedious, but having an ever deteriorating code-base is worse.

Reply on news.ycombinator.com