3 ms·

It sounds like your company has a shitty culture. I've never had issues with code reviews in the companies I worked at. One company was particularly rigorous, w

It sounds like your company has a shitty culture. I've never had issues with code reviews in the companies I worked at. One company was particularly rigorous, where we had to print it ou on paper, and a room of engineers would go over things line by line. But that also had the best engineered software I had seen in my career. Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. There should be a distinction between opinion and technical problems. If you suggest a change that is opinion-based, then it can be taken or ignored. I never comment on suggestions, because it's a waste of my time and theirs. If you are suggesting a chance that is a technical problem, then that should be changed. If the person doesn't want to make the change for the technical problem, then that's a culture problem in the team or company.

autarch 5y ago
> Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. I'd say any issues of style should be caught by required code formatting and linting tools, which optionally run pre-commit and _must_ pass in CI. This should all be handled _before_ code review. Then review can focus on substance, not style.
throwawaythekey 5y ago
> Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. I find that there are two components of style: - Formatting - Clean abstractions, modularization... everything else that goes into making you a great (not just passable) programmer Formatting should be handled by tools like others have said. For everything else I have the options: - Turn the style guide into a book which no one will ready anyway - Ask them to read an actual book but it isn't practical in the scope of a PR, and even after reading the ideas take a while to digest - Attempt to mentor the employee via the PR, often appearing pedantic because it's hard for a novice to appreciate what they don't know. - Code that passes CI is working code - screw everything else! I take the mentoring route whenever it's presented to me but have generally found that some other team members (especially as they get older) have no interest in best practice and become annoyed by the code review process. If the mentee has persistence it tends to work out but moreso over 6mth-1yr timeframes. For mentees without the patience we drop the code review and keep them in areas of the code base where they can't do too much damage. I guess what I'm trying to say is that our culture is willing to bend the processes to fit individual's strengths, and overall our workplace is pretty happy/chill. Maybe the engineering quality takes a hit but I can definitely sympathize with where the OP is coming from.
that_guy_iain 5y ago
> It sounds like your company has a shitty culture. People say this a lot then I look at their code reviews and their code reviews are basically the same. As I said in another comment the root cause of this crappyness is the fact that the majority of people can't code review for crap and they're rushing it. > Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. This doesn't stop people asking. I have a default comment "This is a code style change and as a rule I don't do code style changes unless it's required by the code style guidelines because everyone has their own preferences for code style. If I was to do this change then I would have to do other change and that would result in lots of pull requests being updated just because the specific reviewer wanted it one way and then another reviewer on another pull request wanted it another way." Sometimes then go and have the code style changed stating "Iain won't do code style changes unless they're in the guidelines" which I personally feel is a bitchy comment about the fact they just bikeshedded on my pull request and I wouldn't the change. > If the person doesn't want to make the change for the technical problem, then that's a culture problem in the team or company. Here comes the crux, what if the other person just disagress with your assessment and decides the technical problem doesn't exist or business value of changing it makes it a bad idea to change? For example, in code review I create a table and someone comes a long and says that tables with a large number of columns is a performance issue. I disagree and point out that splitting it out and having multiple tables with joins is a performance issue. Now we have a whole confrontation. Someone points out a valid issue that would affect 1 out of 1 million users and would take 3 days to fix. Again confrontation. Sure you can say with the right culture these issues are easy to work around but the fact is office politics is such a big thing we have a phrase for it which everyone understands. So many people are trying to advance their careers that they all want to be seen as really good. And for most people the way they look good is by making someone else not look good.

autarch 5y ago

> Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. I'd say any issues of style should be caught by required code formatting and linting tools, which optionally run pre-commit and _must_ pass in CI. This should all be handled _before_ code review. Then review can focus on substance, not style.

throwawaythekey 5y ago

> Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. I find that there are two components of style: - Formatting - Clean abstractions, modularization... everything else that goes into making you a great (not just passable) programmer Formatting should be handled by tools like others have said. For everything else I have the options: - Turn the style guide into a book which no one will ready anyway - Ask them to read an actual book but it isn't practical in the scope of a PR, and even after reading the ideas take a while to digest - Attempt to mentor the employee via the PR, often appearing pedantic because it's hard for a novice to appreciate what they don't know. - Code that passes CI is working code - screw everything else! I take the mentoring route whenever it's presented to me but have generally found that some other team members (especially as they get older) have no interest in best practice and become annoyed by the code review process. If the mentee has persistence it tends to work out but moreso over 6mth-1yr timeframes. For mentees without the patience we drop the code review and keep them in areas of the code base where they can't do too much damage. I guess what I'm trying to say is that our culture is willing to bend the processes to fit individual's strengths, and overall our workplace is pretty happy/chill. Maybe the engineering quality takes a hit but I can definitely sympathize with where the OP is coming from.

that_guy_iain 5y ago

> It sounds like your company has a shitty culture. People say this a lot then I look at their code reviews and their code reviews are basically the same. As I said in another comment the root cause of this crappyness is the fact that the majority of people can't code review for crap and they're rushing it. > Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated. This doesn't stop people asking. I have a default comment "This is a code style change and as a rule I don't do code style changes unless it's required by the code style guidelines because everyone has their own preferences for code style. If I was to do this change then I would have to do other change and that would result in lots of pull requests being updated just because the specific reviewer wanted it one way and then another reviewer on another pull request wanted it another way." Sometimes then go and have the code style changed stating "Iain won't do code style changes unless they're in the guidelines" which I personally feel is a bitchy comment about the fact they just bikeshedded on my pull request and I wouldn't the change. > If the person doesn't want to make the change for the technical problem, then that's a culture problem in the team or company. Here comes the crux, what if the other person just disagress with your assessment and decides the technical problem doesn't exist or business value of changing it makes it a bad idea to change? For example, in code review I create a table and someone comes a long and says that tables with a large number of columns is a performance issue. I disagree and point out that splitting it out and having multiple tables with joins is a performance issue. Now we have a whole confrontation. Someone points out a valid issue that would affect 1 out of 1 million users and would take 3 days to fix. Again confrontation. Sure you can say with the right culture these issues are easy to work around but the fact is office politics is such a big thing we have a phrase for it which everyone understands. So many people are trying to advance their careers that they all want to be seen as really good. And for most people the way they look good is by making someone else not look good.