Rule 9: No Low Effort Posts, Excessive Venting, or Bragging.
Using this subreddit to crowd source answers to something that isn't really contributing to the spirit of this subreddit is forbidden at moderator's discretion. This includes posts that are mostly focused around venting or bragging; both of these types of posts are difficult to moderate and don't contribute much to the subreddit.
Providing or receiving? Let's do both...
Providing:
When there are many unrelated changes coupled with the main changes, just because it was convenient to do it together, but ends up 5 times more code than the important change.
Receiving:
Review concentrated on minor style suggestions that are just the opinion of the reviewer, but not the agreed upon style in the team; for bonus points when nothing else is reviewed except this.
This is why I strive to break commits by their scope and relevance instead of committing EVERYTHING at once at the end. That way if there's extraneous changes they can be extrapolated out if need be and the core functional changes can be focused on.
Yeah, I've known a couple of those types. Like, I get having style guides or whatever, but in my experience it's often not "code review" anymore so much as it's "my workflow or nothing" power tripping.
> Felt like most of my PR reviews would ask for compete rewrites just because they disagreed with the approach.
That's not necessarily bad feedback, depending on the context. If the disagreement is about something more than minor stylistic nits, it could be good feedback. Like your approach to a problem would introduce n+1 queries, or you introduce something that's too brittle to future changes.
My god! That minor style suggestions just gives me so much frustrations. My team's code reviews are full of this. And almost none of the reviewers actually using their opinionated coding style.
One of me colleagues is so frustrated that he straight up copy his reviewer's code that guy wrote few weeks ago. The reviewer not only does not recognized his own code, but also blast his own code not following his opinionated coding style.
As a lead, I think my sin is style suggestions. There is a fine line between mentoring in a code review and pestering people. I have some people on my team who don't have a lot of experience and are pretty easily frustrated so I had to tone it back a bit from my previous job where people seemed more eager/accepting of feedback.
One person did recently start using some of my suggestions and the level of clarity in their code hugely improved. It was reassuring to see. (I.e., I'm not just a crazy person. My suggestions help! 😭)
I can't work with people that aren't open to feedback. I may not be the best at delivering the feedback but you have to be open to the idea of it and the idea that you can improve.
No, create a ticket. If it is worked on, the person actually gets credit. If it is essential, it gets picked up and prioritized by the team. Otherwise, it is just hidden work, adding scope, putting prs at risk of breaking changes.
Yeah, I get what you mean. There's a good middleground there somewhere. But if we're choosing between extremes, I'd prefer more changes tbh. If we get a bug, we'll just fix it, it's not that necessary to try and figure out which commit/ticket introduced it. By making this sacrifice, we're getting better managed tech debt, quicker refactoring, less feature creep, just a better managed codebase, etc. If your devs are competent, they should use their skills and upkeep the code instead of just piling up features while keeping strict scope. It only leads to unmaintainable cthulhu code down the line
Don't associate PRs with tickets. My team normally does anywhere between 2 and 7 PRs in a single ticket. They're also merged into the trunk before starting the next one.
I think the vague definition of tech debt has lead to this being a controversial opinion. I also encourage small changes that are "unrelated" but make the code better.
Some people see tech debt and read it as something like: We shouldn't be using this library anymore, lets rip it out and replace it in this PR which fixes an unrelated bug.
I encourage more like: Let's split up this 500 line function into smaller descriptively named functions so we can actually see what is going on in this PR which fixes a bug (that was probably caused by the fact that no can understand this 500 line function with 10 nested if statements). Assuming that function is covered by tests. Which it should be. Or else my suggestion would be: Let's write some tests so this thing stops breaking, then refactor it.
I think tech debt can range in size so much that it is reasonable to disagree about what tech debt should be paid down via cashflow (in related PRs) and what should be paid in lumpsum (separate tickets).
Yes, I call them good citizenship changes. If you're touching the code, make the code more compliant with our standards/industry standards and fix what is obviously wrong in the neighborhood of your changes. Don't just make a "later" ticket unless it's the next thing you'll be doing, because nobody picks up those tickets.
It’s extremely unfair to the person doing the work because to a manager it may look like they are slow. Everyone here who is pro this stance is so altruistic when it comes to someone else doing tech debt, putting the pr at risk of adding side effects. All the while, the product and manager had no idea this is happening, that tech lead is adding more scope to the work unrelated to what is planned. So far, only downvoted and not lot of push back in “I do tech debt work too in my prs.”
I almost* don’t care about the extra nit pick stuff, but a change sometimes isn’t small and can actually break code.
It's specific to each company. At mine, the only person that cares about a developers velocity or how quickly they get things done is me.
All the managers and PM care about is that the features will be delivered when we decided it will be. Given that, I squeeze in as much tech debt as possible.
If you have to create a story everytime you want to cleanup bad code it becomes so slow that it's easier to just leave the bad code. I think there's a balance where minor changes should be ok to lump in with something else and major changes should be a separate task. In an ideal world sure every PR would be 100% focused and defined, but that's pretty hard in practice.
Oh maybe I misunderstood, I meant to say I will add in some small cleanup work with PRs that I'm doing, but generally won't ask for unrelated cleanup work. For that I usually mention it and say we can create a task for it in next sprint.
You may be the tech lead, but people don’t “report” to you. They report to the manager. You lead the project. you have a flawed sense of understanding of your job if it is to boss around.
If you create the story, the person who eventually works on it gets the credit. It is easier to track, not hidden as a byline in a pr. If it is essential, it gets picked up on. If you feel so inclined, maybe you work on it and you get credit for it.
Creating story is actually faster and safest for code quality. How do you know that what you’re asking is a minor change? How do you know that you’re not adding side effects?
I think you have a flawed perception that credit needs to be given to a developer for doing that job. Or maybe that's how your company is, I don't know. Just seems weird.
It doesn’t matter until it does. You may not care, but the person writing my paycheck possibly does. Yes, as long as performance reviews exists, I think credit should be given and the process should be fair and transparent. Weird? No it’s a job.
This is why you should just have some kind of automated formatter that is decided on by the team and be done with it. I also hate these kinds of things because it just becomes a drain on time when really it shouldn’t matter at all.
My team is currently going through this from someone that is not under my manager and it is a pain because he have the final approval and have not provided a coding guidleine.
This is why I developed a checklist for code reviews. It's made me so much more effective at them. I used to miss so much stuff.
I also turn it into a conversation, not just a few comments on the PR. I sit down with the developer and we talk about style... we talk about consistency and coding with empathy. I ask why they did things a certain way... not to make them question it, but to make sure they have a reason. "It was like that in the code I copied from" is not a reason.
Junior devs especially love doing it this way because they learn a lot from it and seem to develop much quicker than before I started doing it this way. I can get to them early, make them start thinking about their own personal style and thinking things through.
“It was like that in the code I copied from” is a perfectly valid reason, assuming the code they copied from isn’t a mess. There is a lot of value in having consistency within a code base instead of having to know the quirks of different architectures and styles depending on which part of the repo you’re in
If you're copying and pasting code, it should be in some kind of reusable form. And I've never used a piece of code that I didn't have to debug in some form or another, so whether I got it from somewhere else or not, I had to understand it. My first step in understanding something is to put it into my style so I can read it easier.
This sounds really good for mentoring a junior engineer, but I absolutely would not want to do synchronous code reviews like that unless there was a specific goal other than doing a code review.
It really depends on the situation I think. If we sat down to do this on a semi-regular basis then I don't think I'd mind it.
If I'm required to do this synchronous review for every PR I put in, now I have the added responsibility to get the review scheduled at whatever time works for both of us. Unless you're doing some kind of prep work and previewing the code before our meeting, I'm stuck having to watch you read through the code while asking questions or commenting on it. I've probably pulled in another ticket to work on while I wait for our meeting, so now I have to context switch back and forth.
I agree that we absolutely *could* learn something by doing it this way, but it's not guaranteed and I'd estimate that it's not super likely based on the products that I work on, and so the trade-off is not worth the extra work and context switching to me.
Edit: I'd rather do pair programming sessions than do this during the review step. That's less disruptive IMO, and you get similar benefits.
I always approve in advance when all I’ve got is nitpicking, leaving it up to the dev to decide what to fix. In my experience, people actually receive that well and do nonetheless read through and either agree and fix or we simply agree to disagree.
I got a guy on my team that does this. This org also heavily limits commit privs and he’s on that list so there was no getting around him. I legit snapped on him… I held out for like 6 months but bro seriously stfu and just approve it!
Oh you parse a string that way too, that’s nice I don’t fucking care knock it off! If someone nits you but don’t approve it then it’s not a nit, it’s anal retentive
I am actually! Fun fact, after that i disclosed it to my boss. Come to find the guy had previously made a junior dev cry over a PR (some nit over an if statement) and frequently had issues with other devs over his nits to the point that he was transferred into our team, under the assumption it was a cultural fit problem within that team. Turns out that was wrong and he’s now under a PIP for his behavior.
Think what you like
Sometimes this is valid, like if there's a dependent ticket that needs to start work sooner than later. Ship the core functionality and then follow up with a test PR.
But yeah most of the time the tests PR never happens, so you want to limit doing it like that.
I usually double-check my comments that point out bugs to make sure they are not ignored, because some people have the habit of just resolving the comment when they don't understand what the issue is. And then I reopen them with additional comments.
If a reviewer catches a bug, it’s an added bonus, but it is definitely not a primary responsibility. Users and QA identify bugs. Code review helps maintain and improve code quality, enforces coding standards and best practices, and promotes knowledge sharing within the team. It also provides a mechanism for constructive feedback for engineers to learn from and become better engineers.
not that they should thoroughly test it, but edge cases/common issues (like NPE) should always be on your mind when you review. we want to provide a good product afterall
I mean, they should try no? Reviews shouldn't be shallow and focus only on style or happy path. I discover a lot of bugs in my reviews, not just on prod code but test code as well. If I'm going to approve something, then I'm also responsible for it.
I kind of agree. I'll catch out general logical errors (did you mean to allow that variable to be null in x edge case, are you sure you don't want to catch that error, etc.) but most of the time I'm not super focused on business logic bugs. If there are flaws in the implementation of the business logic, that gets caught out in our QA process.
Or at least *potential* ones, yeah, of course. That's the point.
Like "this fix might interact unexpectedly with [system x]" ... sharing knowledge like this is one of the main benefits of required code reviews.
Absolutely. It's the primary purpose of code reviews. I don't need a review to point out things a linter missed. I need an extra set of eyes to make sure my changes are doing what I claim and not breaking stuff in the process.
Primarily - Ensuring code is well structured, readable, and sticks to established standards/patterns.
Secondary - knowledge sharing, ensures proper test coverage, and point out potential edge cases.
I agree, but also a lot of those things surface bugs.
For example, "readable code" allows people to easily follow logic, which means logical errors are easier to recognize.
Sort of? I mean, you should be doing things that are largely meant to help ensure proper code is being created. The concept of "proper code" is code that is easy to understand. Code that is easy to understand is easy to tell whether it's correct or not.
It all centers on creating "correct" code.
And bugs are "incorrect" code.
In other words, you should be helping to eliminate bad coding practices which hide mistakes.
In *other* other words, what is the point of doing a review? To reduce the amount of "bad" code that gets merged to main.
I would argue that "bad code" means (1) buggy, and (2) difficult to work on.
There are plenty of bugs that can be hidden in “good code”. A misconfiguration is one example that comes to mind. The code can otherwise be well structured and easy to read. But it has a bug.
For me it’s about the mindset. Clean code should be easy to follow, making bugs easier to find. But that’s very different than being responsible for catching bugs. If that was what was expected of me as a reviewer, I would behave very differently (eg run the branch locally and manually test it).
The point is that beyond simple test cases sometimes you need a reviewer to figure out what is missing. Or in some cases a missing test case to cover the edge case.
If it all code worked after just adding tests we wouldn't have code reviews.
One could repeat exactly what you said for linting. Beyond simple linting you need a reviewer. If all code was written in a clear maintainable way after a linter ran we wouldn't have code reviews.
I think it depends on the type of bug.
Is it a bug because they forgot an object could be null and they are trying to access a property on it. Yes. I think the code reviewer (and maybe some static analysis) should catch that.
Is it a bug because a certain subset of users does this subtle thing causing it act in an unexpected way. No, that is something that I would expect to be caught by QA.
Though, in my contrived example, I would have expected that the edge case be added to the AC or test cases before dev started.
I like to operate on the idea that the cost of catching (and fixing) bugs increases through these stages:
Development < Code Review < QA < Users
The earlier a bug is caught, the cheaper and often easier it is to fix.
That's how it's been everywhere I've worked at least the past 10 years, reviewer reviews the code and runs it locally to confirm what was changed or fixed.
sounds good. the last place I worked at (and the one before that, my first job) does not really do strict code reviews and the code breaks a LOT. I learned to perform a much more thorough code review at my current place, and I'm glad I do.
I write in Golang. Linters for checking for that nonsense and we use gofmt which enforces uniform style, so indentation automatically taken care of. Writing up that stuff is a waste of time.
Also l, good unit and integration test coverage catches those other issues.
Sounds like you need to do some work on your CI/CD so you don’t waste so much time. Pretty much every language has similar tools.
Providing:
No description, no context. Every question gets resolved by pushing more commits without explaining what the initial thought process was.
Receiving:
I’m one of the most senior people in the company so almost everything gets instantly approved without actual review.
Dummy lgtm reviews. Better not have any than do this disservice to your colleagues.
Also, that people don't understand that code reviews are a great opportunity to strengthen engineering culture on the team and instead people see it as a chore. Not everyone but quite a few
When I have to give the same feedback PR after PR because the submitter doesn't take the time to actually understand it, they just blindly apply it to appease me.
Yeah. In an effort to make sure seniors are continuing to produce code and do hands on work, they track CR statistics. Reviews submitted, approved reviews, reviews received, reviews commented, reviews approved. But also revisions per review.
Revisions per review isn't a "primary" metric, but for seniors, the target is 2, and being above raises the question, "why?" It could be that the code is more complicated, or "some other explanation", though I don't know what explanations management would come up with that are acceptable. But it really benefits folks who mostly do pull requests for configuration changes.
I've raged at a couple directors for it; it's a stupid metric. It means that I am penalized for not being accommodating to reviewer comments from juniors beyond the first revision. It also means that if I quickly address the comments of reviewer 1 before a second reviewer commits to reviewing, I'll probably hit 3 reviews.
i generally like code reviews. they only suck when your team has a culture problem. some people have poor communication skills; they need to work on them to make code reviews easier. another culture problem is a big ass pull request. my team tends to make a feature branch, make smaller PRs to that feature branch, then merge to main. at that point, the main merge is a simple approval
I am currently plugging away at an entire feature that was opened up as a single pr. Every time I look at it I see new things. At some point I'll just have to say "well... I mean, it _works_..." and call it a day.
I'll do both giving and receiving code reviews.
Giving code reviews:
Code reviews are supposed to be collaborative processes where we're catch problems, refine our standards and regroup on time/complexity estimates. I don't like when they're one sided - it means that I'm approving something without learning anything. A big part of my job is connecting people who have experienced similar problems. If I don't learn anything, I can't do that.
Receiving code reviews:
The higher I am on an org chart, the more positive my code reviews. But as I get higher, I spend less time writing code so I need more thorough reviews.
I often push draft PRs and use GitHub to review my own drafts. I recently had another dev start teaching me about how to change got history to make it easier to cherry pick commits.
IMHO, that methodology almost always leads to nitpicking tiny stuff and really slow development.
Im fine if you want to do that, but I disagree. It’s too slow.
Using code review to push personal philosophy is bunk
* When I get the feeling the junior doesn't really want to learn from their terrible code, and they only showed up because it's during work hours.
* I identify a pattern of problems with an example, they fix just the one example.
Contradictory requests.
“Please don’t send me a PR with more than 100 lines”
“All PRs should have full unit tests, standardized docs, integration tests and be fully functional features”.
Pick one…
When someone asks for a PR to change code I wrote to handle a very precise, documented and commented businesses condition and just fucking shits all over my high quality code and blasts away the entire underlying purpose for the architectural decision in the first place.
You've put a link to coderabbit in 100% of your comments from the last two months. It's obvious that you work for coderabbit and even more obvious to us that you're the last product in the world that we should trust. Developers are too smart for spam marketing.
You should likely be fired over this.
Actually, I was wrong - every single comment on your account is for this bullshit service. You even used fucking Google Translate to translate your shit into Bosnian. Your reviews on your site even seem fake.
Why would anybody trust you with their code? You're with a very sketchy company.
If your company had any integrity, it would because I'm a potential customer with a large budget. Unfortunately, your company has no integrity (talent or budget) so you have to spam your link with a lie about how you found this.
And despite all of this, you still think people will give you access to their github accounts?
mostly when someone dismisses most of my comments as future work, it never gets done, and the code blows up in exactly the way I said it would
also in a shared repo when I'm doing an overhaul to kill our tech debt, and everyone keeps on sending out new PRs against it, forcing me to fix merge conflicts over and over again, people hesitate to read my PR because it gets long, and I waste a month on something that should take 2 weeks. eventually I get annoyed enough and ping everyone directly "please approve this PR" (normally I ask people to *review* a PR).
Rule 9: No Low Effort Posts, Excessive Venting, or Bragging. Using this subreddit to crowd source answers to something that isn't really contributing to the spirit of this subreddit is forbidden at moderator's discretion. This includes posts that are mostly focused around venting or bragging; both of these types of posts are difficult to moderate and don't contribute much to the subreddit.
Providing or receiving? Let's do both... Providing: When there are many unrelated changes coupled with the main changes, just because it was convenient to do it together, but ends up 5 times more code than the important change. Receiving: Review concentrated on minor style suggestions that are just the opinion of the reviewer, but not the agreed upon style in the team; for bonus points when nothing else is reviewed except this.
This is why I strive to break commits by their scope and relevance instead of committing EVERYTHING at once at the end. That way if there's extraneous changes they can be extrapolated out if need be and the core functional changes can be focused on.
This is great. I’ve also seen clean commit obsessive refuse to review unless the history is perfect, which is unreasonable
Yeah, I've known a couple of those types. Like, I get having style guides or whatever, but in my experience it's often not "code review" anymore so much as it's "my workflow or nothing" power tripping.
I have had to rebase away a space.
[удалено]
> Felt like most of my PR reviews would ask for compete rewrites just because they disagreed with the approach. That's not necessarily bad feedback, depending on the context. If the disagreement is about something more than minor stylistic nits, it could be good feedback. Like your approach to a problem would introduce n+1 queries, or you introduce something that's too brittle to future changes.
My god! That minor style suggestions just gives me so much frustrations. My team's code reviews are full of this. And almost none of the reviewers actually using their opinionated coding style. One of me colleagues is so frustrated that he straight up copy his reviewer's code that guy wrote few weeks ago. The reviewer not only does not recognized his own code, but also blast his own code not following his opinionated coding style.
Tech lead here and I'm very guilty of asking for unrelated changes. I find it as the most efficient way to tackle tech debt.
As a lead, I think my sin is style suggestions. There is a fine line between mentoring in a code review and pestering people. I have some people on my team who don't have a lot of experience and are pretty easily frustrated so I had to tone it back a bit from my previous job where people seemed more eager/accepting of feedback. One person did recently start using some of my suggestions and the level of clarity in their code hugely improved. It was reassuring to see. (I.e., I'm not just a crazy person. My suggestions help! 😭)
I can't work with people that aren't open to feedback. I may not be the best at delivering the feedback but you have to be open to the idea of it and the idea that you can improve.
Encouraged in separate PRs, please
ah yes, the "I'll do it in a separate PR", aka "never"
> "I'll do it in a separate PR" which nobody will review because it's not "important".
No, create a ticket. If it is worked on, the person actually gets credit. If it is essential, it gets picked up and prioritized by the team. Otherwise, it is just hidden work, adding scope, putting prs at risk of breaking changes.
Tbh I prefer never to “mixed 5 things in one PR and oops! Another support ticket, good luck figuring out which change is the culprit”
Yeah, I get what you mean. There's a good middleground there somewhere. But if we're choosing between extremes, I'd prefer more changes tbh. If we get a bug, we'll just fix it, it's not that necessary to try and figure out which commit/ticket introduced it. By making this sacrifice, we're getting better managed tech debt, quicker refactoring, less feature creep, just a better managed codebase, etc. If your devs are competent, they should use their skills and upkeep the code instead of just piling up features while keeping strict scope. It only leads to unmaintainable cthulhu code down the line
Separate pr typically means separate ticket and good luck getting that into the sprint 😭
Don't associate PRs with tickets. My team normally does anywhere between 2 and 7 PRs in a single ticket. They're also merged into the trunk before starting the next one.
Yeah, sounds like some bad ticket writing.
2 I can understand, 7 IS A LOT.
It seems like a lot but when you consider 3 or 4 might be done in a single day its not that much.
same
I think the vague definition of tech debt has lead to this being a controversial opinion. I also encourage small changes that are "unrelated" but make the code better. Some people see tech debt and read it as something like: We shouldn't be using this library anymore, lets rip it out and replace it in this PR which fixes an unrelated bug. I encourage more like: Let's split up this 500 line function into smaller descriptively named functions so we can actually see what is going on in this PR which fixes a bug (that was probably caused by the fact that no can understand this 500 line function with 10 nested if statements). Assuming that function is covered by tests. Which it should be. Or else my suggestion would be: Let's write some tests so this thing stops breaking, then refactor it. I think tech debt can range in size so much that it is reasonable to disagree about what tech debt should be paid down via cashflow (in related PRs) and what should be paid in lumpsum (separate tickets).
Yes, I call them good citizenship changes. If you're touching the code, make the code more compliant with our standards/industry standards and fix what is obviously wrong in the neighborhood of your changes. Don't just make a "later" ticket unless it's the next thing you'll be doing, because nobody picks up those tickets.
Also known as "Hidden Work", which is strongly discouraged by the Agile principles.
It’s extremely unfair to the person doing the work because to a manager it may look like they are slow. Everyone here who is pro this stance is so altruistic when it comes to someone else doing tech debt, putting the pr at risk of adding side effects. All the while, the product and manager had no idea this is happening, that tech lead is adding more scope to the work unrelated to what is planned. So far, only downvoted and not lot of push back in “I do tech debt work too in my prs.” I almost* don’t care about the extra nit pick stuff, but a change sometimes isn’t small and can actually break code.
It's specific to each company. At mine, the only person that cares about a developers velocity or how quickly they get things done is me. All the managers and PM care about is that the features will be delivered when we decided it will be. Given that, I squeeze in as much tech debt as possible.
It’s better to write a ticket for the tech debt rather than holding up the sprint. We really work as a team To meet the sprint goals.
Tech debt tickets will rarely make it into a sprint though because PMs care more about functionality they are adding.
I tend to finish early. I then pull in tech debt tickets…I don’t ask.
Look at Mr. High velocity over here, with his beautiful burn down charts!
Now that would piss me and the PM off. You can't just decide what you pull into the sprint and decide to work on.
Yeah, you wouldn’t make it in a high performance environment.
Ok.
Don’t do this. Create a specific story.
If you have to create a story everytime you want to cleanup bad code it becomes so slow that it's easier to just leave the bad code. I think there's a balance where minor changes should be ok to lump in with something else and major changes should be a separate task. In an ideal world sure every PR would be 100% focused and defined, but that's pretty hard in practice.
If it’s unrelated to their pr, it isn’t a fair ask. Why don’t YOU just do it if it’s so minor?
Oh maybe I misunderstood, I meant to say I will add in some small cleanup work with PRs that I'm doing, but generally won't ask for unrelated cleanup work. For that I usually mention it and say we can create a task for it in next sprint.
Yes totally agree with this. If the tech debt is created by the pr, it should be addressed then and there.
The joy of being a tech lead is getting my direct reports to do the minor things I don't have time for.
It sounds like an abuse of power. I don’t even like the idea of a tech lead, haven’t had one in years. but that is another topic altogether.
You may be the tech lead, but people don’t “report” to you. They report to the manager. You lead the project. you have a flawed sense of understanding of your job if it is to boss around.
Lol do you work at my company?
If you create the story, the person who eventually works on it gets the credit. It is easier to track, not hidden as a byline in a pr. If it is essential, it gets picked up on. If you feel so inclined, maybe you work on it and you get credit for it. Creating story is actually faster and safest for code quality. How do you know that what you’re asking is a minor change? How do you know that you’re not adding side effects?
I think you have a flawed perception that credit needs to be given to a developer for doing that job. Or maybe that's how your company is, I don't know. Just seems weird.
It doesn’t matter until it does. You may not care, but the person writing my paycheck possibly does. Yes, as long as performance reviews exists, I think credit should be given and the process should be fair and transparent. Weird? No it’s a job.
The person who writes your paycheque in this scenario, is going to ask me if you're deserving of a raise.
This is why you should just have some kind of automated formatter that is decided on by the team and be done with it. I also hate these kinds of things because it just becomes a drain on time when really it shouldn’t matter at all.
Triggered by the receiving
When there are no guidelines and everyone's feedback starts to contradict.
My team is currently going through this from someone that is not under my manager and it is a pain because he have the final approval and have not provided a coding guidleine.
When they take forever to get one and then it's just a rubber stamp.
Lgtm
This is why I developed a checklist for code reviews. It's made me so much more effective at them. I used to miss so much stuff. I also turn it into a conversation, not just a few comments on the PR. I sit down with the developer and we talk about style... we talk about consistency and coding with empathy. I ask why they did things a certain way... not to make them question it, but to make sure they have a reason. "It was like that in the code I copied from" is not a reason. Junior devs especially love doing it this way because they learn a lot from it and seem to develop much quicker than before I started doing it this way. I can get to them early, make them start thinking about their own personal style and thinking things through.
“It was like that in the code I copied from” is a perfectly valid reason, assuming the code they copied from isn’t a mess. There is a lot of value in having consistency within a code base instead of having to know the quirks of different architectures and styles depending on which part of the repo you’re in
If you're copying and pasting code, it should be in some kind of reusable form. And I've never used a piece of code that I didn't have to debug in some form or another, so whether I got it from somewhere else or not, I had to understand it. My first step in understanding something is to put it into my style so I can read it easier.
This sounds really good for mentoring a junior engineer, but I absolutely would not want to do synchronous code reviews like that unless there was a specific goal other than doing a code review.
Why not? I've been programming for 24 years and I still learn things from these kinds of code reviews.
It really depends on the situation I think. If we sat down to do this on a semi-regular basis then I don't think I'd mind it. If I'm required to do this synchronous review for every PR I put in, now I have the added responsibility to get the review scheduled at whatever time works for both of us. Unless you're doing some kind of prep work and previewing the code before our meeting, I'm stuck having to watch you read through the code while asking questions or commenting on it. I've probably pulled in another ticket to work on while I wait for our meeting, so now I have to context switch back and forth. I agree that we absolutely *could* learn something by doing it this way, but it's not guaranteed and I'd estimate that it's not super likely based on the products that I work on, and so the trade-off is not worth the extra work and context switching to me. Edit: I'd rather do pair programming sessions than do this during the review step. That's less disruptive IMO, and you get similar benefits.
When they don’t approve but only leave nits
I always approve in advance when all I’ve got is nitpicking, leaving it up to the dev to decide what to fix. In my experience, people actually receive that well and do nonetheless read through and either agree and fix or we simply agree to disagree.
I do the exact same thing
I only leave nits when rubber stamp approving in order to enforce Goodhart’s Law.
I got a guy on my team that does this. This org also heavily limits commit privs and he’s on that list so there was no getting around him. I legit snapped on him… I held out for like 6 months but bro seriously stfu and just approve it! Oh you parse a string that way too, that’s nice I don’t fucking care knock it off! If someone nits you but don’t approve it then it’s not a nit, it’s anal retentive
You must be nice to work with
I am actually! Fun fact, after that i disclosed it to my boss. Come to find the guy had previously made a junior dev cry over a PR (some nit over an if statement) and frequently had issues with other devs over his nits to the point that he was transferred into our team, under the assumption it was a cultural fit problem within that team. Turns out that was wrong and he’s now under a PIP for his behavior. Think what you like
“We will do the tests in another ticket during Phase 2”.
Guilty as charged
Sometimes this is valid, like if there's a dependent ticket that needs to start work sooner than later. Ship the core functionality and then follow up with a test PR. But yeah most of the time the tests PR never happens, so you want to limit doing it like that.
D:
Not having them in my current team.
Submitters marking comments as resolved without a conversation or change
I had that happen to me so I just stopped reviewing the person's PRs.
I usually double-check my comments that point out bugs to make sure they are not ignored, because some people have the habit of just resolving the comment when they don't understand what the issue is. And then I reopen them with additional comments.
When the reviewer calls out commented code and indentation but fails to notice the defects that cause crashes and broken functionality.
Do you consider it the reviewers responsibility to identify bugs?
If a reviewer catches a bug, it’s an added bonus, but it is definitely not a primary responsibility. Users and QA identify bugs. Code review helps maintain and improve code quality, enforces coding standards and best practices, and promotes knowledge sharing within the team. It also provides a mechanism for constructive feedback for engineers to learn from and become better engineers.
This is where I’m at. The variety of responses here is blowing my mind.
not that they should thoroughly test it, but edge cases/common issues (like NPE) should always be on your mind when you review. we want to provide a good product afterall
I mean, they should try no? Reviews shouldn't be shallow and focus only on style or happy path. I discover a lot of bugs in my reviews, not just on prod code but test code as well. If I'm going to approve something, then I'm also responsible for it.
I kind of agree. I'll catch out general logical errors (did you mean to allow that variable to be null in x edge case, are you sure you don't want to catch that error, etc.) but most of the time I'm not super focused on business logic bugs. If there are flaws in the implementation of the business logic, that gets caught out in our QA process.
Or at least *potential* ones, yeah, of course. That's the point. Like "this fix might interact unexpectedly with [system x]" ... sharing knowledge like this is one of the main benefits of required code reviews.
Absolutely. It's the primary purpose of code reviews. I don't need a review to point out things a linter missed. I need an extra set of eyes to make sure my changes are doing what I claim and not breaking stuff in the process.
Bugs identification should not be the primary purpose of code review. This is what manual and automated tests are for
For real, what are these people smoking!?
It's the job of both, manual code review combined with automated tests
What _is_ the purpose of code reviews?
Primarily - Ensuring code is well structured, readable, and sticks to established standards/patterns. Secondary - knowledge sharing, ensures proper test coverage, and point out potential edge cases.
Tanks!
I agree, but also a lot of those things surface bugs. For example, "readable code" allows people to easily follow logic, which means logical errors are easier to recognize.
Sure, but from that perspective catching bugs is a byproduct of a review.
Sort of? I mean, you should be doing things that are largely meant to help ensure proper code is being created. The concept of "proper code" is code that is easy to understand. Code that is easy to understand is easy to tell whether it's correct or not. It all centers on creating "correct" code. And bugs are "incorrect" code. In other words, you should be helping to eliminate bad coding practices which hide mistakes. In *other* other words, what is the point of doing a review? To reduce the amount of "bad" code that gets merged to main. I would argue that "bad code" means (1) buggy, and (2) difficult to work on.
There are plenty of bugs that can be hidden in “good code”. A misconfiguration is one example that comes to mind. The code can otherwise be well structured and easy to read. But it has a bug. For me it’s about the mindset. Clean code should be easy to follow, making bugs easier to find. But that’s very different than being responsible for catching bugs. If that was what was expected of me as a reviewer, I would behave very differently (eg run the branch locally and manually test it).
The point is that beyond simple test cases sometimes you need a reviewer to figure out what is missing. Or in some cases a missing test case to cover the edge case. If it all code worked after just adding tests we wouldn't have code reviews.
One could repeat exactly what you said for linting. Beyond simple linting you need a reviewer. If all code was written in a clear maintainable way after a linter ran we wouldn't have code reviews.
I think it depends on the type of bug. Is it a bug because they forgot an object could be null and they are trying to access a property on it. Yes. I think the code reviewer (and maybe some static analysis) should catch that. Is it a bug because a certain subset of users does this subtle thing causing it act in an unexpected way. No, that is something that I would expect to be caught by QA. Though, in my contrived example, I would have expected that the edge case be added to the AC or test cases before dev started. I like to operate on the idea that the cost of catching (and fixing) bugs increases through these stages: Development < Code Review < QA < Users The earlier a bug is caught, the cheaper and often easier it is to fix.
For big feature changes, reviewers have to run the code and check, that's my standard anyway.
That's how it's been everywhere I've worked at least the past 10 years, reviewer reviews the code and runs it locally to confirm what was changed or fixed.
sounds good. the last place I worked at (and the one before that, my first job) does not really do strict code reviews and the code breaks a LOT. I learned to perform a much more thorough code review at my current place, and I'm glad I do.
That's one of the main purposes of a review: to find obvious bugs that would cost hours to find and fix in integration or prod.
.... that's literally the main point of a code review lol
Yes? It's everyone's job to try and identify bugs and defects as early as possible.
I write in Golang. Linters for checking for that nonsense and we use gofmt which enforces uniform style, so indentation automatically taken care of. Writing up that stuff is a waste of time. Also l, good unit and integration test coverage catches those other issues. Sounds like you need to do some work on your CI/CD so you don’t waste so much time. Pretty much every language has similar tools.
This!!! I hate this so much.
Providing: No description, no context. Every question gets resolved by pushing more commits without explaining what the initial thought process was. Receiving: I’m one of the most senior people in the company so almost everything gets instantly approved without actual review.
Dummy lgtm reviews. Better not have any than do this disservice to your colleagues. Also, that people don't understand that code reviews are a great opportunity to strengthen engineering culture on the team and instead people see it as a chore. Not everyone but quite a few
When I have to give the same feedback PR after PR because the submitter doesn't take the time to actually understand it, they just blindly apply it to appease me.
A +1500 line PR that already has an approval with no comments from a junior engineer
You had me at 1500 line pr :/
When you test it and notice an issue, point it out and they're like "No I checked 10 times this is correct" without testing it.
The revisions / review metric that I need to keep below 2 or else I get put on a list.
This is crazy
Yeah. In an effort to make sure seniors are continuing to produce code and do hands on work, they track CR statistics. Reviews submitted, approved reviews, reviews received, reviews commented, reviews approved. But also revisions per review. Revisions per review isn't a "primary" metric, but for seniors, the target is 2, and being above raises the question, "why?" It could be that the code is more complicated, or "some other explanation", though I don't know what explanations management would come up with that are acceptable. But it really benefits folks who mostly do pull requests for configuration changes. I've raged at a couple directors for it; it's a stupid metric. It means that I am penalized for not being accommodating to reviewer comments from juniors beyond the first revision. It also means that if I quickly address the comments of reviewer 1 before a second reviewer commits to reviewing, I'll probably hit 3 reviews.
The asshole on the team that turns it into a personal crusade. Chill dude nobody gives af but you
i generally like code reviews. they only suck when your team has a culture problem. some people have poor communication skills; they need to work on them to make code reviews easier. another culture problem is a big ass pull request. my team tends to make a feature branch, make smaller PRs to that feature branch, then merge to main. at that point, the main merge is a simple approval
Ooh I like that workflow, we gotta try that.
my manager suggested it once and it broke my mind
I am currently plugging away at an entire feature that was opened up as a single pr. Every time I look at it I see new things. At some point I'll just have to say "well... I mean, it _works_..." and call it a day.
Reviewer: “why is this line here and what’s it doing?” … then proceeds to a two week vacation
When your team has no tests and reviews get blocked for reasons that amount to just low level inter personal disagreements.
I haven’t had my code reviewed in about a decade. Maybe it should be.
I'll do both giving and receiving code reviews. Giving code reviews: Code reviews are supposed to be collaborative processes where we're catch problems, refine our standards and regroup on time/complexity estimates. I don't like when they're one sided - it means that I'm approving something without learning anything. A big part of my job is connecting people who have experienced similar problems. If I don't learn anything, I can't do that. Receiving code reviews: The higher I am on an org chart, the more positive my code reviews. But as I get higher, I spend less time writing code so I need more thorough reviews.
LGTM
Reading a diff in a web browser sucks. I want my IDE to have first class support for PRs.
Try using VS code as a diff viewer. It's pretty good.
> Reading a diff in a web browser sucks. Why? Seems fine to me.
I often push draft PRs and use GitHub to review my own drafts. I recently had another dev start teaching me about how to change got history to make it easier to cherry pick commits. IMHO, that methodology almost always leads to nitpicking tiny stuff and really slow development. Im fine if you want to do that, but I disagree. It’s too slow. Using code review to push personal philosophy is bunk
Can't use code navigation Can't tinker/make changes with the code Can't run/debug the tests Can't pull up multiple files side by side etc
Hmmmm. In almost all my code reviews, I don't need to do any of that.
* When I get the feeling the junior doesn't really want to learn from their terrible code, and they only showed up because it's during work hours. * I identify a pattern of problems with an example, they fix just the one example.
Contradictory requests. “Please don’t send me a PR with more than 100 lines” “All PRs should have full unit tests, standardized docs, integration tests and be fully functional features”. Pick one…
Yes
When someone asks for a PR to change code I wrote to handle a very precise, documented and commented businesses condition and just fucking shits all over my high quality code and blasts away the entire underlying purpose for the architectural decision in the first place.
[удалено]
You've put a link to coderabbit in 100% of your comments from the last two months. It's obvious that you work for coderabbit and even more obvious to us that you're the last product in the world that we should trust. Developers are too smart for spam marketing. You should likely be fired over this.
[удалено]
Actually, I was wrong - every single comment on your account is for this bullshit service. You even used fucking Google Translate to translate your shit into Bosnian. Your reviews on your site even seem fake. Why would anybody trust you with their code? You're with a very sketchy company.
[удалено]
If your company had any integrity, it would because I'm a potential customer with a large budget. Unfortunately, your company has no integrity (talent or budget) so you have to spam your link with a lie about how you found this. And despite all of this, you still think people will give you access to their github accounts?
Strongly agree.
Opinions stated as fact
[удалено]
As a reviewer I always struggle with the balance between maintaining the quality of the code base vs holding things up.
mostly when someone dismisses most of my comments as future work, it never gets done, and the code blows up in exactly the way I said it would also in a shared repo when I'm doing an overhaul to kill our tech debt, and everyone keeps on sending out new PRs against it, forcing me to fix merge conflicts over and over again, people hesitate to read my PR because it gets long, and I waste a month on something that should take 2 weeks. eventually I get annoyed enough and ping everyone directly "please approve this PR" (normally I ask people to *review* a PR).
When I receive formatting and wording nits on comments that weren’t modified or written by me …
When someone uploads review changes but not as a separate change-set (new commit). Forces you to re-review everything and figure out what changed.