Senior engineer is making PR review a nightmare
Posted by thewritingwallah@reddit | ExperiencedDevs | View on Reddit | 53 comments
Has anyone dealt with a developer who is out to get you on PR reviews? he will create discussions for every tiny thing with often little benefit. For the sake of an example variable naming causing lengthy discussions. Every approach i take he will argue for almost the exact opposite and its tiring. I realise that this brings benefit and rubber stamping them without thought is also bad but has anyone been in a similar situation before or can offer some guidance?
He does this to other devs often not backing down on minor disagreements where either way would work and im sensing its causing the team morale to suffer.
so many code review antipatterns I can see by him
- add a ransom note and hold the change
- he plays guessing game and criticise my particular solution, on some grounds that don’t relate to whether or not it solves the problem.
- he start reviewing the code, add a review comment pointing it out and stop reading and send me back, I fix and send him back again and this become like 'Thousand Round Trips'
- sometimes he send his friend and make a double team and like 'One patch. Two reviewers.'
and so many other code review antipatterns.
Im 16yoe working in a startup environment as a consultant.
What has been the most effective way for you to perform code reviews in your teams?
pl487@reddit
You're a consultant. The senior engineer is your customer. Discuss the tradeoffs explicitly: if we establish standards for these things so I understand what you want, I can do them the right way the first time, and we are spending a lot of my time doing the PR dance. But if this is the way they prefer to work, it is what it is.
Mammoth-Clock-8173@reddit
This is really insightful. Years ago I worked with a very good consultant who was really laid back when our architect made him redesign a solution for the 3rd time. When asked how it wasn’t pissing him off, he replied, “My responsibility is to deliver the architect’s vision, not manage the project. CompanyX is paying me by the hour. If they think the best use of their money is to have me do this 5 times, it’s not my role to second guess.”
logafmid@reddit
A lot of devs want to dismiss that code reviews can be sources of toxicity. Some people see them as an opportunity to raise their profile by looking authoritative by making others change things. They aren't really looking to make the code better (some are far to cargo-culty to even hope to make the code better). It's the same thing you see when executives or business people all need to make inane demands to look involved.
dbalatero@reddit
I think the biggest problem in code review is not clarifying whether a comment matters or not. I use https://conventionalcomments.org to make it really clear when something is blocking or non-blocking. I try to leave thorough comments (whatever catches my eye), but most comments in practice are non-blocking. My rule is if all my comments are non-blocking I have to give the approval stamp.
To me this is the best blend of:
Izacus@reddit
Why would you make a comment if it doesn't matter? It's just noise then, isn't it?
dbalatero@reddit
Comments are an opportunity to spark discussion, share opinions, and offer up ideas to teammates. Sometimes they land and resonate, and sometimes they do not. They are not always blocking.
Izacus@reddit
Aren't PRs a bit late to have those kind of discussions though? I mean, if the thing is important enough to point out, it should be blocking until discussion is resolved. If it's not important enough to care to block on it, why waste time of the author?
dbalatero@reddit
It sounds like you think sharing thoughts or information with team members is a "waste of time" so I'm not sure what I'm going to convince you of here. Are you a slow reader?
Izacus@reddit
PRs aren't the time and place to share thoughts though? The code is already written then?
dbalatero@reddit
Sure, the earlier a conversation the better, but:
No matter how many times I encourage people to have earlier and earlier conversations, I cannot force them and they don't do it all the time for various reasons
Some conversations are about the code itself, so I don't get how you'd have an early conversation
Early conversations are not perfect
I think you have a misconception that I'm leaving like 50 comments on a PR or something. It's more on the order of 0 to 8, whether or not they are blocking or non blocking.
Sheldor5@reddit
a PR is the last thing in a feature/bugfix lifecycle (code wise) so its later than too late ...
Choperello@reddit
No it isn't. Merging code comes after. So is deploying it. So is having to fix whatever bugs it may have introduced. So is having to keep maintaining it. A PR is by no means the "last" thing in the software life cycle, nor even the most energy consuming one.
No_Structure7185@reddit
its not. because the person will produce more code in the future. so its a good way to give them ideas how to improve their code. even if it wont be included in the code of the current PR.
Choperello@reddit
So what if its written. It's not merged, it's not deployed. Just because "it has been written" doesn't make it locked in stone, nor automatically good, or etc.
throwaway_0x90@reddit
That wasn't necessary.
dbalatero@reddit
true
Choperello@reddit
I mean, how are you supposed to point something about how a PR does things until you see it? By your logic, if the PR author choose not to discuss every single detail before they send out the PR, too late everyone should just STFU.
throwaway_0x90@reddit
There are times where things can be "This is fine for now, but in the future.."
Usually I see people make indicate that kind of intention by putting "nit: blah blah blah" as the comment.
"Nit", for ESL people, short for "nitpick" - meaning the person making the comment knows their comment could be considered annoying and not worth the effort given the current context, but the comment's author still feels it's important to inform the code's author and anyone else.
Especially senior eng probably feel obligated to not allow bad habits to go unchecked.
theBenForce@reddit
Thanks for sharing the conventional comments page! I use conventional commits all the time, so using this format for comments makes sense.
dbalatero@reddit
IMO, I think the most important piece of it is the blocking/non-blocking tag. Categorizing the comment (nit, suggestion, etc) is a nice extra piece of information, but table-stakes is clearly communicating how much someone should care about a piece of feedback, so they can triage and spend their time accordingly.
thewritingwallah@reddit (OP)
The problem becomes that it costs you time, now you have to update your PR with a bunch of silly changes. 50 PRs later that adds up. How long do you put up with that?
IsseBisse@reddit
Maybe you need to take the time to write down a style-guide for your code.
To me, a big part of code review is aligning the team. A new (to the team) developer might get a lot of comments in the beginning, but eventually they learn from these comments, adapt their style and the amount of comments drop.
However, that only works if you have a "correct" style in your team. That could be a style-guide or a team lead with final say (given that their code style is consistent). Preferably both.
throwaway_0x90@reddit
Yeah, I've actually changed teams because of that once. I personally felt it was getting to the point that I wouldn't be able to meet my expectations for the next performance eval if every PR I make requires a month of academic exercise to defend.
Another way to handle this is to create a design doc and get this Senior person making comments in your doc as early as possible, with code-snippets here 'n there. Also try looking at the code they commit and get a feel for their style and try to copy it.
I can't say if doing this is "right" or "wrong", but if your goal is to just get your job done and your own code-style isn't a hill you need to die on defending, then you can just "blindly do whatever the senior says" and get on with your day using this strategy.
FooBarBuzzBoom@reddit
I had a colleague that had OCD. Just try to ignore him and talk to your PM. Worst case, change your job.
Educational_Pea_4817@reddit
does your team have retros? if so maybe bring up the fact that your team doesnt abide by specific code standards that make it pull requests hard since you dont know what you should be doing.
Saki-Sun@reddit
Names are important. They are the primary way a developer communicates with the reader.
PixelsAreMyHobby@reddit
Agreed, but those things should be defined in a style guide.
worst_protagonist@reddit
The style of naming is in the style guide. You can follow the style and still choose poor names
PixelsAreMyHobby@reddit
I think naming is super important, but that’s also why it helps to have conventions written down in a style guide.
Otherwise you just end up re-arguing the same stuff in every PR.
For example:
That way nobody has to guess what’s “right” and reviews can focus on the actual logic instead of whether it should be editable or isEditable…
Saki-Sun@reddit
A style guide can't cover the nuances of naming things. Getting names correct and concise is often not easy.
e.g. EmailHelper.SendEmail(id) which publishes a request to a message queue to send a pre-saved email. A style guides going to struggle with that level of laziness.
LogicalPerformer7637@reddit
I had similar issue with a senior developer randomly picking pull requests for review. He has choosen mine for a library refactoring. He did not like the way I defined the API without any real arguments. I was sucessful to shut him up by listing features/advantages of my approach and then asking how to do it better if he does not like my approach. He did not respond and I have peace from him since.
andrey-r@reddit
This is 🤌. When merits win. Not ego size. Hats off to both of you really.
failsafe-author@reddit
Need examples- otherwise, have to assume you just need to make the changes. “Variable naming” is in the table for reasons to block a code review.
theBenForce@reddit
While not exactly the same, I worked with someone that would review my code and give tons of comments on coding style. I'd push back and point out that they were just stylistic changes. He'd approve the MR then go back later and rewrite it.
I think part of the problem was we were at a really small startup with a lot of opinionated junior/mid level devs. The place that I work at now has a pull-request reviewer guide that explicitly says coding style changes are considered `nits` and shouldn't block approval.
m98789@reddit
The senior engineer may be on some form of stimulant that causes them to hyper focus on small details.
Choperello@reddit
Tbh I am also way more picky with consultants. In my experience most of them want to just take the fastest path to closing the project so they can start billing the next client, without really caring about the maintainability of what they leave behind, or their changes being consistent with what’s already there. No idea if this also the case here but it’s a thing.
mafiazombiedrugs@reddit
Am consultant, can confirm, please just lgtm it and move on so I can work on something else.
/j, I understand that the customer has to use what I make and have no problem with them triple checking my work, but some people def don't get that.
Beatlepoint@reddit
Since you are 16 he probably has more responsibility for your prs than you do, and giving him the benefit of the doubt he might be trying to teach you. The only example I noted was variable names, which are important and can be worth lengthy discussions. My one question is whether you and him should spend more time discussing the plan before you even raise a pr.
Shinroo@reddit
OP isn't 16, OP has 16 years of experience.
Beatlepoint@reddit
Oh, "I am 16 years of experience" through me off.
mafiazombiedrugs@reddit
Yoe is years of experience my guy
Hot-Hovercraft2676@reddit
Even if you have 100 YOE some idiots can still challenge something silly: Can we add an empty line here? Can we rename “customer” to “buyer”? Can we make this string (which we are using it once only) a constant? Can we move the constant to a new file…?
Sensitive-Ear-3896@reddit
Yep my favorite is addressing the 15 review comments only for them to “find” 5 more
ClassyCamel@reddit
If this is actually causing an issue with team productivity and morale then it’s squarely in the responsibility of your/their manager to address and resolve
Thommasc@reddit
> For the sake of an example variable naming causing lengthy discussions
That's why there can only be one team lead. The team lead will say if it's A or B. End of discussion. Moving on with your life.
It's totally fine to complain a lot in PR reviews, this is the place for such discussion. You just need to make sure there is an efficient way to move forward and get it merged when it's in the right spot.
At the end of the day it's all about productivity and business requirements.
Devs should follow the business, not dictate it. On the long term this will kill the product.
thewritingwallah@reddit (OP)
This is basically what I do.
For little changes that don't matter I just make them. I don't even bother to reply, just give the comment a thumbs up when I've made the requested change and close the thread.
CorrectRate3438@reddit
I can remember back in the day, we’d have code review in a meeting. It was annoying for different reasons but possibly worth considering.
rayeranhi@reddit
I’m just adding a comment because I have a guy like this on my team and it’s exhausting. Scope creep, 500+ required nits that are unrelated to the PR. I feel your pain.
Hot-Hovercraft2676@reddit
There is exactly the same guy in my team. He even suggested approach A two months ago then suggest approach B now, where approaches A and B are just syntax level things.
I told my manager and he just asked me to changes the code in his way if it doesn’t take too much time, which means he isn’t very interested in the issue. Thankfully, I am leaving the company soon, but I have no idea what I could do if I were to stay.
Fair_Local_588@reddit
Is this person new to the team or are you new to it? I find that people often over-analyze PRs in these cases either to “prove” their value or show authority (respectively). I just went through this so not sure how to fix it, but I empathize with you.
Zorrette@reddit
I don't have the full context but maybe tell him ? I have a friend who IS BEYOND ANNOYING on little details, like everything need to be explain, discussed and variable naming can be talked about for literal hours. At some point I just start telling him when it's too much, or when we don't really care. He's a very good friend now, because we tell each other when it's important and not.
You can also put reviews rules, like read everything at each review, guidelines for the commands etc etc.
If it's not enough and if he's reviews are made just to be a pain in everybody a**, I would say to gather everybody sentiment and pass it to his boss, force in number.
Shinroo@reddit
Hard to give feedback without actual examples
thewritingwallah@reddit (OP)
Honestly I usually let it go and agree if it’s not a substantive change. It’s not worth my time to argue with him. he will ultimately ostracize and undermine himself with the whole team. I'm experienced, I know this but i hope it’s ok to vent from time to time.