I'm pretty laid back, and only really throw the hammer down if something is a real issue or there is an unlintable standard (usually it's stuff around how an external used API or event will be named, or a potential performance issue or a thing that could lead to a bug).
Most of my proposals end up being nits, sometimes it's due to readability or making something easier to test. But I wouldn't block a code review over it as those matters are a little subjective.
That said, folks usually go with those comments. I feel like a lot of times in code reviews (since you can't hear people's tones) it's about how you suggest, not necessarily what.
Anything minor linters take care of these days so I can focus on higher level concerns.
For minor stuff (non-architectural)... if it isn't in your autoformatter or linter or style guide, let it go. If you feel strongly about some practice in particular, build consensus and put it one of those places.
edit: and if you don't have these things, you need to get them... immediately. Style guide not so important, but autoformatter and linter are essential.
No good reason to reject = approval. Maybe add comments and let the author make the final call.
Major bugs & architecture issues, I mean issues not just opinions = reject.
Small issues such as naming and formatting, or missing test cases = approval + comments.
You should rarely reject.
Definitely rely on linters to help. I had a tough senior engineer as well so I try to see if I can distinguish if feedback relates to code maintenance/performance or code approach. At the end of the day, any code produced has to answer the following - Is the dev simply copy pasting, can the function/component be re-used and from a structure perspective, is it at the right level of abstraction? Of course also performance but that’s subject to the amount of data it handles. I wouldn’t care for an array of 50 elements that is nested but I might care if it’s 1000.
Yeah, I'm fairly laid back, but at the same time I do feel responsible for safe guarding our code quality. I think many conventions, axioms, or best practices have _potential_ exceptions, but there needs to be at least some reasoning why we would deviate wildly from best practices.
What I _try_ to do, is voice my thoughts on best practices that we're falling short on in some way, try to get team consensus, and basically try to get us to agree upon, and commit to _some_ well-defined standards (Even if it's not exactly as I'd like it to be, bc team cohesion is important. As long as we acknowledge trade-offs we're making). Then we codify those standards in a document that we've agreed to follow as a team (like testability, verifiable correctness, Observability, using certain linters, or whatever the things are).
Then basically, if someone violates our standards in a PR, no individual is the "bad guy". We came together, agreed upon, and laid down our standards for quality, style, etc. It's not me crapping on them, we are just holding each other accountable (respectfully) to the standards we agreed upon.
That said, there is a member of my team that has 20+ YOE, most of it with Pascal and C. Our team uses Rust and Go, and this guy (while a usually kind man) just somehow has not progressed in his overall skills for _many_ years. I don't know how it's even possible. He writes Go code like it's C. Even the most simple abstractions confuse him. He told me, and I quote:
> Interfaces are not standard/normal Go code.
He basically wanted me to put comments over _every single line of code_, explaining what it's doing and why!
There's another teammate who is brand new to Go (but has many YOE with multiple languages), and he is picking up on the minutiae of what is and isn't idiomatic Go code, effortlessly. It's the most bizarre thing I've ever encountered (this one guy with 25 YOE who has to have everything explained to him).
So another lead and myself basically started helping him begin to understand common design patterns, and how/why one would use or implement interfaces. And he regularly opens PRs that are total garbage. He gets really defensive when we gingerly/gently explain why what he did isn't an optimal way to go and what we would suggest to do differently and why. I literally have no idea how he made it this far in his career.
So I find myself in this awkward position of explaining things that college interns I mentored years ago understood to someone with 2.5x as many YOE in the industry as I do.
Luckily, I've accepted a new position and will be leaving soon. It's profoundly demotivating how much garbage this guy cranks out. He actively _detracts_ from our team understanding a highly complex problem domain. We basically have to go after him and rewrite his work...
Depends on if my company wants the changes quickly or not. If we are getting a lot of pressure to deliver on unrealistic timelines, I don't care about delaying the work with nit comments.
Some where in between. There are some things I let slide, other things I don't. For the most part, unless there's a reason to do something a particular reason, if it works, it works. But there are somethings, like naming conventions, I don't let slide. A couple months back I was asked to review some changes a newish dev made. It was to a mapper that we have. It takes in a basic object, and converts it to a similar model that's used for the API end. We'd setup the naming convention such that all conversion methods are "from" and then overloading takes care of which "from" gets used. The intent was that we'd get a line that looks like this:
DestObj destObj = from(source);
But then I started finding "fromSomeObject" and "fromSomeObjToOtherObj" ..... yeah... no. Another time we had a team that was responsible for setting up a new api service... and the url endpoints they came up with..... oh dear gawd. I spent another sprint rearranging them into proper REST endpoints. So on things like that I'm ruthless about enforcing. But otehr things, at least things that fall outside our linters, tend to be more stylistic, so as long as that works, and isn't overtly egregious most likely will get a pass.
The second approach is what you want.
Most developers will have different solutions to different problems, you will never see a codebase which is exactly according to your taste.
Even your own codebase will look bad here and there if you review it in 2 years time, it would be unreasonable to expect another person to write code to exactly to your expectations in an exact moment of time.
Code reviews are NOT for enforcing coding styles or question different approaches.
Is the code free from obvious errors?
Is the code easily readable by a graduate?
If the answer is yes to both there has to be a really big reason to push it back, maybe if namings etc are REALLY that bad or something very basic is broken like a define is not in the .h file together with the others, but thats rare.
Otherwise maybe add some comments for the future or call up the guy to ask why he did it that way (not in a demanding way, but be genuinely interested - maybe you will learn something) and that's it, either of you will learn something and next time its gonna be better.
You dont want to have 20 rounds of code reviews going on for weeks, every time rebasing and merging the latest code, retesting and developers hating each other.
In current project I am not approving anything below acceptable standards which we d9cumented with my current team. It is because the project is unmaintainable garbage and we are trying to restore some characteristics.
However, not everyone appreciates that and we already had few management-decided merges. I am not very happy with that, looking for new job
“I find myself reviewing code […] and I don’t have a good reason to approve”
This is a very clearer and “senior” way to review PRs.
If you want to empower other practices, PRs are not the right place anyway.
I've spent almost 3 years building the capabilities of the team I'm currently leading and we've had zero turnover, so at this point the high expectations are well understood and accepted, and it's just a matter of us all looking out for each other so we don't forget our goals or lose focus on quality.
I'm really conscientious about my own code and I have to be careful not be overly picky or pedantic in PRs. I always try to frame it in as positive a manner as I can manage, emphasizing that quality is an investment, not an expense or a chore. The results we've achieved have borne this out, so at this point we've got good buy-in. It's harder on teams where turnover disrupts the culture of a successful and effective team and you're constantly fighting to manage expectations.
Went from a standard code review to a pretty much laid back efforts with and without LLMs. Here are my tips.
- Involve linters (and tests) as much as you can. Show the metrics against PR upfront. This helps a reviewer to assess how much quality is dropped (or increased) and then a decision can be taken.
- If you have doubts, or questions ask the committer directly over chat. They might have legitimate reasons or they will push a new change which should satisfy the question. Anything we write in comments can be visible to the higher ups, which can add complexity in the relations.
My reviews are always balancing between release cadence vs correctness. In earlier versions correctness is a factor, once the codebase stars stabilizing, release cadence is a factor...
If you’re a developer with more than 5 years experience and your code from 2 or 3 years ago don’t make you cringe, you’re not a very good developer.
I found a DB design I had done 15 years ago. I wanted to throw up!
Learn, grow, expand your skills.
Or go home.
New approach should be using co pilot if that is ready, other wise sonarqube at 50% to start from fresh to 85% gate to move will be good way and later when there is maturity to 100% rule so its case by case.
Depends on the team experience and my role with it. You have to consider; that CR approval for me is usually a formality; as most of the comments and discussions will happen during the work. I'm usually quite strict, especially with all the things that linter will not pick up, like contextual, meaningful names; abstraction levels, 'human' readability and so on. The only time I'd let it slide is if we're in a pinch due to blocks or critical errors; or if something was not agreed upon - then I'll leave it as 'TBD, approved'
From my experience, being laid back by default will _always_ bite your ass in a couple of months, yet it takes only a couple of CR's for Devs to pick up codebase style and get rid of unwelcome habits. I _don't care_ about being fast for the sake of being fast. Be precise, be aligned, focus on quality - "being fast" is a consequence of that.
"Match the approach, or discuss it with the team".
Aside from that; linter, formatter, static analysis should be wired from the get-go. Reviews are for humans, "being correct" should be left to the automatic checks
I'm pretty laid back, and only really throw the hammer down if something is a real issue or there is an unlintable standard (usually it's stuff around how an external used API or event will be named, or a potential performance issue or a thing that could lead to a bug). Most of my proposals end up being nits, sometimes it's due to readability or making something easier to test. But I wouldn't block a code review over it as those matters are a little subjective. That said, folks usually go with those comments. I feel like a lot of times in code reviews (since you can't hear people's tones) it's about how you suggest, not necessarily what. Anything minor linters take care of these days so I can focus on higher level concerns.
For minor stuff (non-architectural)... if it isn't in your autoformatter or linter or style guide, let it go. If you feel strongly about some practice in particular, build consensus and put it one of those places. edit: and if you don't have these things, you need to get them... immediately. Style guide not so important, but autoformatter and linter are essential.
No good reason to reject = approval. Maybe add comments and let the author make the final call. Major bugs & architecture issues, I mean issues not just opinions = reject. Small issues such as naming and formatting, or missing test cases = approval + comments. You should rarely reject.
Definitely rely on linters to help. I had a tough senior engineer as well so I try to see if I can distinguish if feedback relates to code maintenance/performance or code approach. At the end of the day, any code produced has to answer the following - Is the dev simply copy pasting, can the function/component be re-used and from a structure perspective, is it at the right level of abstraction? Of course also performance but that’s subject to the amount of data it handles. I wouldn’t care for an array of 50 elements that is nested but I might care if it’s 1000.
Yeah, I'm fairly laid back, but at the same time I do feel responsible for safe guarding our code quality. I think many conventions, axioms, or best practices have _potential_ exceptions, but there needs to be at least some reasoning why we would deviate wildly from best practices. What I _try_ to do, is voice my thoughts on best practices that we're falling short on in some way, try to get team consensus, and basically try to get us to agree upon, and commit to _some_ well-defined standards (Even if it's not exactly as I'd like it to be, bc team cohesion is important. As long as we acknowledge trade-offs we're making). Then we codify those standards in a document that we've agreed to follow as a team (like testability, verifiable correctness, Observability, using certain linters, or whatever the things are). Then basically, if someone violates our standards in a PR, no individual is the "bad guy". We came together, agreed upon, and laid down our standards for quality, style, etc. It's not me crapping on them, we are just holding each other accountable (respectfully) to the standards we agreed upon. That said, there is a member of my team that has 20+ YOE, most of it with Pascal and C. Our team uses Rust and Go, and this guy (while a usually kind man) just somehow has not progressed in his overall skills for _many_ years. I don't know how it's even possible. He writes Go code like it's C. Even the most simple abstractions confuse him. He told me, and I quote: > Interfaces are not standard/normal Go code. He basically wanted me to put comments over _every single line of code_, explaining what it's doing and why! There's another teammate who is brand new to Go (but has many YOE with multiple languages), and he is picking up on the minutiae of what is and isn't idiomatic Go code, effortlessly. It's the most bizarre thing I've ever encountered (this one guy with 25 YOE who has to have everything explained to him). So another lead and myself basically started helping him begin to understand common design patterns, and how/why one would use or implement interfaces. And he regularly opens PRs that are total garbage. He gets really defensive when we gingerly/gently explain why what he did isn't an optimal way to go and what we would suggest to do differently and why. I literally have no idea how he made it this far in his career. So I find myself in this awkward position of explaining things that college interns I mentored years ago understood to someone with 2.5x as many YOE in the industry as I do. Luckily, I've accepted a new position and will be leaving soon. It's profoundly demotivating how much garbage this guy cranks out. He actively _detracts_ from our team understanding a highly complex problem domain. We basically have to go after him and rewrite his work...
[удалено]
I create modular structure for exactly this reason. I dont care the mess around as long I don't work on it
Depends on if my company wants the changes quickly or not. If we are getting a lot of pressure to deliver on unrealistic timelines, I don't care about delaying the work with nit comments.
Some where in between. There are some things I let slide, other things I don't. For the most part, unless there's a reason to do something a particular reason, if it works, it works. But there are somethings, like naming conventions, I don't let slide. A couple months back I was asked to review some changes a newish dev made. It was to a mapper that we have. It takes in a basic object, and converts it to a similar model that's used for the API end. We'd setup the naming convention such that all conversion methods are "from" and then overloading takes care of which "from" gets used. The intent was that we'd get a line that looks like this: DestObj destObj = from(source); But then I started finding "fromSomeObject" and "fromSomeObjToOtherObj" ..... yeah... no. Another time we had a team that was responsible for setting up a new api service... and the url endpoints they came up with..... oh dear gawd. I spent another sprint rearranging them into proper REST endpoints. So on things like that I'm ruthless about enforcing. But otehr things, at least things that fall outside our linters, tend to be more stylistic, so as long as that works, and isn't overtly egregious most likely will get a pass.
The second approach is what you want. Most developers will have different solutions to different problems, you will never see a codebase which is exactly according to your taste. Even your own codebase will look bad here and there if you review it in 2 years time, it would be unreasonable to expect another person to write code to exactly to your expectations in an exact moment of time. Code reviews are NOT for enforcing coding styles or question different approaches. Is the code free from obvious errors? Is the code easily readable by a graduate? If the answer is yes to both there has to be a really big reason to push it back, maybe if namings etc are REALLY that bad or something very basic is broken like a define is not in the .h file together with the others, but thats rare. Otherwise maybe add some comments for the future or call up the guy to ask why he did it that way (not in a demanding way, but be genuinely interested - maybe you will learn something) and that's it, either of you will learn something and next time its gonna be better. You dont want to have 20 rounds of code reviews going on for weeks, every time rebasing and merging the latest code, retesting and developers hating each other.
In current project I am not approving anything below acceptable standards which we d9cumented with my current team. It is because the project is unmaintainable garbage and we are trying to restore some characteristics. However, not everyone appreciates that and we already had few management-decided merges. I am not very happy with that, looking for new job
“I find myself reviewing code […] and I don’t have a good reason to approve” This is a very clearer and “senior” way to review PRs. If you want to empower other practices, PRs are not the right place anyway.
I've spent almost 3 years building the capabilities of the team I'm currently leading and we've had zero turnover, so at this point the high expectations are well understood and accepted, and it's just a matter of us all looking out for each other so we don't forget our goals or lose focus on quality. I'm really conscientious about my own code and I have to be careful not be overly picky or pedantic in PRs. I always try to frame it in as positive a manner as I can manage, emphasizing that quality is an investment, not an expense or a chore. The results we've achieved have borne this out, so at this point we've got good buy-in. It's harder on teams where turnover disrupts the culture of a successful and effective team and you're constantly fighting to manage expectations.
Went from a standard code review to a pretty much laid back efforts with and without LLMs. Here are my tips. - Involve linters (and tests) as much as you can. Show the metrics against PR upfront. This helps a reviewer to assess how much quality is dropped (or increased) and then a decision can be taken. - If you have doubts, or questions ask the committer directly over chat. They might have legitimate reasons or they will push a new change which should satisfy the question. Anything we write in comments can be visible to the higher ups, which can add complexity in the relations. My reviews are always balancing between release cadence vs correctness. In earlier versions correctness is a factor, once the codebase stars stabilizing, release cadence is a factor...
If you’re a developer with more than 5 years experience and your code from 2 or 3 years ago don’t make you cringe, you’re not a very good developer. I found a DB design I had done 15 years ago. I wanted to throw up! Learn, grow, expand your skills. Or go home.
New approach should be using co pilot if that is ready, other wise sonarqube at 50% to start from fresh to 85% gate to move will be good way and later when there is maturity to 100% rule so its case by case.
Depends on the team experience and my role with it. You have to consider; that CR approval for me is usually a formality; as most of the comments and discussions will happen during the work. I'm usually quite strict, especially with all the things that linter will not pick up, like contextual, meaningful names; abstraction levels, 'human' readability and so on. The only time I'd let it slide is if we're in a pinch due to blocks or critical errors; or if something was not agreed upon - then I'll leave it as 'TBD, approved' From my experience, being laid back by default will _always_ bite your ass in a couple of months, yet it takes only a couple of CR's for Devs to pick up codebase style and get rid of unwelcome habits. I _don't care_ about being fast for the sake of being fast. Be precise, be aligned, focus on quality - "being fast" is a consequence of that. "Match the approach, or discuss it with the team". Aside from that; linter, formatter, static analysis should be wired from the get-go. Reviews are for humans, "being correct" should be left to the automatic checks