I am on a new team learning a new language and new frameworks, I can use all of the code review I can get. I need feed back, before I put a bunch of garbage into the code base. Code review is needed for new team members.
I’m the middle guy, but I’m a masochist, so:
The lead on my team is very much a LGTM reviewer. The project has been going on for 6-7 years.
This is how you get thousands of defects at worst and multiple engineers time worth of tech debt at best.
We have a mantra that “If it’s good enough to merge, it’s good enough for production”. With the intention of automatic production deployment sometime in the future.
That’s has helped us improve quality, a lot.
Currently going through this at work and yes, I’m the guy in the middle. Feeling something between furious anger, maximum frustration, some sadness and worry about my future. Not in IT though.
I went through this at my last job. I am a firm advocate of good comments in code, and my work was being reviewed by a guy 25 years younger than me who was a firm advocate of the idea that comments in code are not only undesirable but also an actual “code smell” - evidence of the professional incompetence of the developer that put them there.
I’m a school bus driver now.
What code is doing should generally (exceptions are always possible) be evident from the code itself, through clear naming, concise functions with singular goals and proper code structure.
Why code is doing what it does can be helpfully explained through a comment. X may update Y because of business decision Z, so putting a little bit of background info on Z can be very helpful for a future maintainer who might understand what X is doing but might not know why it’s doing so in the first place.
If your code requires a lot of comments to make sense, the comments certainly can be a code smell.
Yesterday I got feedback on my PR asking me to add more XML comments. The userId parameter of type Guid parameter didn’t have a param tag that said that it was the ID of the user. I was unable to convince them this was unnecessary and eventually conceded by adding a comment userId: ID of user.
See, that to me is an example of a useless comment. The only way I’d justify it is if you have some validation that checks if all public methods have xml comments or something. But userId: ID of user sounds redundant to me.
I agree with what you say (and that is exactly what I meant by “good comments”), but that is not what this fellow’s philosophy was. His philosophy was that all comments are bad no matter what, with no exceptions. In his code reviews, he simply excised all comments, regardless of why they were there and what purpose they served.
That sounds boneheaded. Comments are a feature for a good reason after all.
Shit. I could translate that into my area of expertise. And simply being older than the rest.
And yes, I got fired. Not sure yet what I’m going to do next. This area feels poisoned now.
