Have you ever had to deal with code reviewers that have absolutely no idea how to do it and instead use the chance to go on an ego trip?
My company has no formal code review process, just that it has to be "reviewed" by somebody, which makes it very political and bureaucratic to get some code in. "Reviewers" which are your peers would block your changeset for days on complete nonsense like:
1. "your code is not wrong, but this is how I would do it, can you please change?"
2. "I don't like this variable named "statusCode", can you name it "statusCodeInt"?"
3. "I know this is not your code but can you also make some changes to it as well?"
4. "passing int by value is too slow, can you pass it by reference?"
5. "I see you need to return a pointer, can you take a double pointer and make it a void function?"
6. "I know this is existing code but I don't understand it, can you add more comments here?"
7. "can you refactor this whole codebase while you're at it?"
8. "what if `new Object()` returns null? can you add a null check?"
If you don't do as they say then your changeset will never get merged. How should I do the needful and revert back properly in this case?
- New / EngpHXp44Have standards that are agreed upon. If people agree that integer variables should have “Int” as a suffix, make that a rule. Better yet, make it a rule that a linter can verify automatically. Same goes for returning pointer vs. double pointer or even pass by value vs. pass by reference. Don’t let things devolve into opinion debates. The specific decision is less important than whether or not you can have an agreed upon rule that everyone follows. If you do that, code reviews will be much more productive.
- This is what I use: what you are saying makes sense, but it seems orthogonal to the problem I am trying to solve. How about we circle back to this in a later PR?
- Amazon donut-It has been a while since I did C++, but I recall the possibility of getting null from a new call could be worth checking for, esp if you have a valid retry strategy. But otherwise, yeah, I guess it’s pointless whether you catch and throw an exception vs having the program throw and crash.
- Google come2daddyGoogle has concept of readability. Only people with a language readability can do formal code reviews. And getting readability is months long process as one is expected to read many many many pages of style guide and books .
This helps in maintaining consistency in reviews. Maybe it is time to start selling consistent coding style across Bloomberg. Easiest would be to adopt existing standards and then adapt it through a council.
- 7) is an example of major scope creep, and should not be necessary. Simply state that it's out of scope for the ticket, and create a follow-up ticket to refactor
- Microsoft / EngbebK16If you can do small refactorings as you go, you can avoid big refactoring. Sometimes the ask really is out of scope, in which case a "won't fix" response is perfectly acceptable. But sometimes it's totally worth doing, "This test class is now tens of thousands of lines long with hundreds of methods. Move your new methods either to a more appropriate test class or create a new one. And then take a minute to look at other related tests and cut & paste then while you're at it."
- i would leave honestly. this is a broken culture and nearly impossible to fix as an IC complaining on blind.
- Some of my excuses:
1. Outside the scope of this PR.(PULL REQUEST).
2. File a work item for future work, assign it to team lead to prioritize. Mark created by field to the reviewer who suggested it.
3. Be vocal when someone is steam rolling you, and get everyone in the team to discuss. With the new beehive cubicle arrangements that's the easiest to do.
Having said that, I have definitely seen some so called Champs going on ego trips.
- VMware nsxlifeGood time for you to start creating the formal code review process you want for your team.
Tell your team that code reviews are taking too long if that’s the case with everyone.
Define the standards, document it, take inputs and share.
- Exactly. Reframe the argument this way: this is a reasonable tech debt to take on, in order to ship the product / bug fix sooner. There must be someone in your company who wants the product to ship sooner rather than later. (It may be your boss or a project manager.) Make an ally of him / her.
- 1. Ask to explain why.
2. If that is your team's/orgs convention, follow it
3. Sure, why note if it's not risky. Leave the code better than you found it.
4. Probably NBD, but probably no harm.
5. Not sure about this one. Can someone from BDE chime in?
6. Good way of learning. Commenting code you are unfamiliar with is a common and easy entry point.
8. Welcome to C++.
- Bloomberg Sgvp47I have had so many instances like this. Reviewers want the code to be like how they would have written it. Sometimes it will take up like 3-4 weeks to get a simple code review done.
I have given up now, i would suggest to just copy paste from the review and get it over with.
- Bloomberg / EngWPslayermoreLol @ 4.
In my department, the cod reviews aren’t like that. I discuss strategy with teammates prior to embarking on the code change. The times I’ve been asked to change things, the changes have been constructive.
Perhaps you should bring these concerns about code reviews up with your manager or skip levels during 1:1 with some concrete examples of unconstructive reviews on how they prevented you from being more productive.
- I have seen instances where managers themselves will tell you to be not afraid to push back during code reviews, since people tend to hold others to a higher bar, compared to themselves.
At the same time managers/TLs will refrain from getting involved in heated debates on reviews, rather than putting a foot down and establishing standards.Mar 120
- I have commented on this in other questions. Part of code review is building a skill that lets you gracefully deal with difficult people.
You will also find that a team that “democratizes” code reviews sabotage good work. Because it only take one dissenter to prevent a pull request from being accepted.
- Two Sigma bingshuhThe worst I've ever had in code reviews is fundamental design disagreements between me and a reviewer; eventually those just are resolved by polling the team, going with whoever has more experience with the area, and maybe manager fiat. They are annoying but the process is necessary to produce good code.
On the other hand, I've done code reviews where the author is super stubborn about every little change, and seems to think their code is perfect from the start, and has to be merged instantly, with no time for revisions. I dunno OP, if those examples are how you view most code review feedback, maybe you need to be a little less defensive about your code. Are you just posting to get justification for your refusal to change anything in code reviews?
- Amazon / EngfHUg57The problem isn't limited to smaller companies. As prestigious as it is, Amazon has the same problem. Egos are bigger than you think. I have one team member who has double standards and argues for everything and eventually flip the argument and make you look like a fool. She doesn't add test cases at all and always creates a ticket to add test cases later, which she'll never get to.