If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.
A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of making them prove their mettle.
I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.”
Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there.
I agree that it’s good to mentor someone new, but honestly I think they still make most decisions themselves, and sometimes those decisions don’t match established patterns that they don’t know about. Ideally you notice sooner than a PR but I also think that people get busy and it’s ok to not have time to monitor everything a new person does. So sometimes it comes down to the code review to notice.
It’s not derogatory or hurtful, literally at all, it’s just pointing on that they did something in a way that goes against established patterns, and it’s teaching them what those patterns are.
But seriously, I'd ask why, and why again. I'm very much against cargo culting, and I will refuse to make changes in my PR if it is cargo culting. You're welcome to open a PR to my PR with changes if you feel strongly about it though.
Maybe this makes me hard to work with, but so far, I feel like it has led to better code and a higher velocity, everywhere I've worked.
Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns.
What you write is not YOUR code. It is your TEAM'S code, and the good of the team is far more important than what you personally like.
I also disagree with it being "the team's code":
What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means).
The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.
It's not like you're being told to never use for loops. Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against:
* This logic should live in [other component] * Our RPCs are named as GetFoo, not DetermineFoo * That function name does not make it obvious what this code does, please change it * This needs a test * This is untestable and needs to be refactored to support X
If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.
Same. Nor do I consider code review comments personal attacks, but anything that must be changed for "reasons," must be questioned. Not because it's personal, but because I legitimately want to know and I won't give up until I get a good answer.
> Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against
I'll agree that none of this is probably worth pushing back against, but it isn't objective. Very little is objective, in our industry.
> This logic should live in [other component]
Why? DDD will say one thing, MVC will say something different, though usually they are compatible or can be made to be compatible. You can also subscribe to hexagonal architectures and that might say something different...
There's nothing objective about where code should live, except on a hard drive or some other storage medium. I would argue that code probably shouldn't live on paper. I imagine most of us would agree with that.
> Our RPCs are named as GetFoo, not DetermineFoo
This is a nit. I'd honestly probably ignore it.
> That function name does not make it obvious what this code does, please change it
I'd probably ask for a suggestion because it probably looks obvious to me after staring at the code for so long. That being said, it might be a valid suggestion, especially if the code was refactored, but wasn't renamed.
> This needs a test
I hope nobody ever says this on my code reviews. However, I don't write tests for "obviously correct" code (code where the test implements the logic to test the logic): such as a function like:
function returnTrue(): true { return true; }
> This is untestable and needs to be refactored to support X
Do you know there is a such thing as legitimately untestable code (or at least, it shouldn't be tested in traditional unit tests)? Usually at the edges of two systems. For example, an API integration can't be tested fully, only the known contract from the other system. Then you start running into Postel's Law ... things get weird. Only if you have some kinds of guarantees with the other system (not usually), would I recommend traditional tests.
>This is a nit. I'd honestly probably ignore it.
Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused.
"The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers.
As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.
Do they not have PR's where you are from?
If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa.
The code is mutable. It doesn't have to be perfect on the first iteration.
I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.
Once you land RPC DescribeFoo, I have to:
1) Define a second parallel RPC GetFoo
2) Wait for it to roll out to prod
3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover
4) Wait for all of those to roll out
5) Clean up all the callers
6) Clean up the DescribeFoo RPC
.....or you could just fix it in code review with half an hour of work.
Anyway. To me, DetermineFoo doesn't sound like a getter and seems like it might even possibly mutate some states. So, I suspect creating a GetFoo would be orthogonal to deprecating DetermineFoo. This means DetermineFoo might only Get a Foo by side-effect. That's why I would probably ignore it; simply because DetermineFoo might be a better name, in my opinion.
New behaviors occur all the time in software, but it doesn't mean we need to be rigid with naming things.
I wasn't sure if I was going to reply to this part or not. But you took a comment out of context here.
I'm incredibly touchy about code reviews. I used to have the popular opinion here where "the code belongs to the team!" and all that. And then I was stuck on a team with a very toxic guy who weaponized this stuff. His code was "perfect" and he'd approve your PR to turn around and rewrite it just to show how much better it could be (and gloat about it -- not realizing if he hadn't deleted half the tests, they'd be failing). He'd constantly point out all the things that were "wrong" and with the justification of "code quality" without actually being able to point out why it was better. After working with them for two years, the entire team's philosophy turned into the one I've shared here ... because it can't be weaponized. It can't be used to tyrannically rule the code. It can't be used to bully people around "for the sake of quality!"
His "code quality tyranny" did more damage to the code than any actually bad code could have done.
So yeah, I will go into a war of attrition over certain kinds of comments that are spoken authoritatively yet are entirely opinionated ... because I've seen what happens when it gets out of hand. I have no desire to go through that ever again. Ever. If that means someone like you fires me for it, it's better than going through that shit again. Sorry, not sorry.
I'm also starting to think this is how the Paradox of Tolerance happens. I'm intolerant of intolerance these days. I'm much more tolerant of people having their way of doing things than I was just 10 years ago. However, as soon as one person comes along and says "It MUST be this way because QUALITY!" I go into intolerance mode and don't tolerate it.
Now, if you can give me a reason grounded in computer science, or whatever paradigm we're using (DDD, hexagonal arch, actor-model, etc), then I'm perfectly happy to agree or spend a few minutes over a coffee discussing merits. But if the reason is 'quality' ... it's a "dual to the death."
I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority.
Here is the most recent PR I did for a relatively young person in my org. Part of the PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly.
I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing".
Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?
1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that).
2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better?
3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.
In this particular case:
Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library.
If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it.
The work has already been done. Try to capitalize on it, instead of dismissing it.
Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.
Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted.
The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.
Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.
To be honest, I’m not sure how you got there from here…
> The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.
I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart.
So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.
"I've done all this work, get put of my way and submit" is the attitude in both cases.
Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with.
The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.
That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review.
I don’t know how else to say that at this point.
> The default answer to code review feedback is always "sure" unless you have a strong argument otherwise.
I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer.
A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.
Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in.
> The work has already been done. Try to capitalize on it, instead of dismissing it.
Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that.
> Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.
If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass?
Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn.
My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions.
I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.
If your team is insulting you or saying that you're a bad engineer based on code review comments, that's a bad team. That doesn't mean that ensuring new team members learn the team's style and patterns is hazing.
If you don't suck it up, you're not "one of us" and you need to go. Or maybe they told you to do the wrong thing and see if you point it out. Who knows!?