Ask HN: Styling comments in PR reviews
8 comments
Usually it’s one of three things:
>Your style isn’t up to spec
If you always smell crap, check the bottom of your shoes. I’ve found team style guides to be useless generally, but attempting to emulate team style by looking at existing code is a winning move. Coding is a team effort, and even if it’s a little extra work, a little beautification can go a long way towards readability.
>Code reviewers want or are incentivized to help
Many code reviewers make a point not to leave a nontrivial review without commenting. Whether this is because they want to help, or because in many workplaces, the code review metric optimized for is “comments closed”, some people will feel obligated. When there’s no substance to dissect, style is often substituted
>Someone’s giving you a hard time for irrational reasons
This is an interpersonal issue. You don’t, in my opinion, need to worry how others will view you if this happens since most people can tell when somebody is just being a dick in a CR. If you’re concerned about this, try to figure out why you two have beef by asking them and either resolve it, ignore/forgive it and move on (some hills aren’t worth dying on), involve management or HR, or change roles
>Your style isn’t up to spec
If you always smell crap, check the bottom of your shoes. I’ve found team style guides to be useless generally, but attempting to emulate team style by looking at existing code is a winning move. Coding is a team effort, and even if it’s a little extra work, a little beautification can go a long way towards readability.
>Code reviewers want or are incentivized to help
Many code reviewers make a point not to leave a nontrivial review without commenting. Whether this is because they want to help, or because in many workplaces, the code review metric optimized for is “comments closed”, some people will feel obligated. When there’s no substance to dissect, style is often substituted
>Someone’s giving you a hard time for irrational reasons
This is an interpersonal issue. You don’t, in my opinion, need to worry how others will view you if this happens since most people can tell when somebody is just being a dick in a CR. If you’re concerned about this, try to figure out why you two have beef by asking them and either resolve it, ignore/forgive it and move on (some hills aren’t worth dying on), involve management or HR, or change roles
Yes very valid points.
I am thinking of making flashcards with all the styling comments and really internalise them. Its like failing a driving license test everytime I create a pull request. haha...
Anyways, I was keen on knowing if this is a common problem (say like 10% of the population faces or its even scarcer).
I personally have never seen a project fail because the styling was a little off. I have seen projects succeed because they hit the customer early, and fail because they didn't do it a lot better than existing solutions. But then, to be honest, every project has its own flavours to success.
I am thinking of making flashcards with all the styling comments and really internalise them. Its like failing a driving license test everytime I create a pull request. haha...
Anyways, I was keen on knowing if this is a common problem (say like 10% of the population faces or its even scarcer).
I personally have never seen a project fail because the styling was a little off. I have seen projects succeed because they hit the customer early, and fail because they didn't do it a lot better than existing solutions. But then, to be honest, every project has its own flavours to success.
Forget notecards, what you need is a well configured linter. There's no reason to save style issues for code review when they can be caught with static analysis. Having encountered this team issue previously, linting made it a non-issue. Pre-commit hooks and CI are two options for enforcing code style.
Some people need to comment on something in order toi justify their existance. Subjective issues are the easiest.
I assume PR is not public relations?
<story> Long time ago I worked at this place where the CEO was known for needed to fel involved without actually being involved. My boss had me work on a demo that the CEO would use for potenial clients. In the middle of the demo it did some odd steps. Clearly a mistake, I had just failed to remove a part in the scripting. Well I showed this to my boss, with the odd part in, since I did not realize it was there. When it happened I commented that I obviously need to remove that part. He told me to leave it in, as it would give the CEO something to complain about, and feel like he had made a real contribution. When shown to the CEO he loved the presentation except for that one glitch. You could almost read his mind ("oh my what would these foolish developers do without me"). <story off>
Sadly I have seen this repeated many times.
I assume PR is not public relations?
<story> Long time ago I worked at this place where the CEO was known for needed to fel involved without actually being involved. My boss had me work on a demo that the CEO would use for potenial clients. In the middle of the demo it did some odd steps. Clearly a mistake, I had just failed to remove a part in the scripting. Well I showed this to my boss, with the odd part in, since I did not realize it was there. When it happened I commented that I obviously need to remove that part. He told me to leave it in, as it would give the CEO something to complain about, and feel like he had made a real contribution. When shown to the CEO he loved the presentation except for that one glitch. You could almost read his mind ("oh my what would these foolish developers do without me"). <story off>
Sadly I have seen this repeated many times.
This is a really interesting story. Glad you shared it. I suppose, being a CEO, one is entitled to such privileges. :)
PR = pull request. Created so that your peers can review the code and catch any issues (architectural, logical or just styling). I think the value attributed to them should be 100x to 10x to 1x (or lesser than 1x).
But its not a commonly held belief. Many software engineers take pride on producing beautiful code. Its not about the product but also about wanting to be the best (say taking it from 99% to 99.9%). But it has its bad side - product can take a back seat to the new cool aid drink language on the block.
PR = pull request. Created so that your peers can review the code and catch any issues (architectural, logical or just styling). I think the value attributed to them should be 100x to 10x to 1x (or lesser than 1x).
But its not a commonly held belief. Many software engineers take pride on producing beautiful code. Its not about the product but also about wanting to be the best (say taking it from 99% to 99.9%). But it has its bad side - product can take a back seat to the new cool aid drink language on the block.
So back in the dark ages ... 20 years ago, company I worked for had what they called 'SoftWare Intensive Inspections'. Code we were writting delt with Telecomm and if a system failed then the telephone company lost money and when it failed real bad, 911 would not work and people could get hurt.
Part of the proces was that when you reviewed stuff, you could comment on: 1) does not work 2) not understandable 3) not maintainable 4) failed to meet standard - but these were usually related to #2 or #3 5) excessively slow or large - like could be written to be 10 times faster, just double the speed was a STFU
During the inspection the author was the note taker. Another would read the code and explain it as best they could. If they could not explain the code, then it failed #2 & #3. One person, usually team lead would arbitrate if the problem was really an issue.
This process seemed to work real well, and found a lot of bugs before the code was fielded. Of course someone would always argue the tabs verses spaces, and where do the curly braces go. This was usually met with STFU. The most common standard fail was the use of magic numbers. I had one jerk who actually did a #define TEN 10. Needless to say that failed.
In all of this STYLE was never a valid complaint.
Part of the proces was that when you reviewed stuff, you could comment on: 1) does not work 2) not understandable 3) not maintainable 4) failed to meet standard - but these were usually related to #2 or #3 5) excessively slow or large - like could be written to be 10 times faster, just double the speed was a STFU
During the inspection the author was the note taker. Another would read the code and explain it as best they could. If they could not explain the code, then it failed #2 & #3. One person, usually team lead would arbitrate if the problem was really an issue.
This process seemed to work real well, and found a lot of bugs before the code was fielded. Of course someone would always argue the tabs verses spaces, and where do the curly braces go. This was usually met with STFU. The most common standard fail was the use of magic numbers. I had one jerk who actually did a #define TEN 10. Needless to say that failed.
In all of this STYLE was never a valid complaint.
Use Prettier and never look back.
And tslint ... <-- does not work so great on my vscode for some reason.
(mostly looking for interesting stories... out of the closet)
(mostly looking for interesting stories... out of the closet)
Is it common for one or two people on a team to receive a lot of styling comments? I am pretty sure this is a good mix of product focussed and pure software engineering focussed groups.
Drop a paragraph on what story or insight comes to your mind on the subject.