How deep do you go in code reviews?
Posted by JobSightDev@reddit | ExperiencedDevs | View on Reddit | 56 comments
As of right now, when I do code reviews for my jr devs, I'm just making sure there aren't any big red flags in the code.
I'm not really going very deep in the logic or design and if there are potential errors in these. I assume the dev did testing enough to make sure it works and I'll let the testing team find anything else that might pop up.
How deep do you go in your reviews?
CalmTheMcFarm@reddit
I review all PRs with the same set of considerations:
Part of this comes from working in international teams, where we had to be able to communicate intent clearly. This was not just for colleagues doing new development which might depend on my changes, but also for sustaining the product into the future. The round-trip time to ensure the sustaining team understands what is going on can add considerable latency to support ticket resolution, so if we put the effort in to making things clear prior to initial integration then we're not just being kind to future us, we're also saving the company money. The money saving is more of an issue when you want to prove to finance that you're worth keeping. YMMV.
I also do live code reviews with new and junior staff, so that I can talk through my process, show them what I am concerned with (immediately and then after some reflection), and ensure that I build them up. I trust them to do the right thing, but I verify that they have as well.
Yes, I've done PR reviews where I've sent back dozens of comments about grammar and writing style - in code, messages and READMEs. I've also sent reviews back with "I cannot approve this at all, because (collection of reasons)". In those cases I then line up time to work directly with the developer to find out how they got to that point as well as setting them on the correct path. Those cases are quite rare, though, because when we have daily standups it's a lot easier to keep my finger on the pulse and my teams know that they can come to me at any time to check an idea.
Finally - you mention design. I generally don't have an issue with design flaws because when we're scoping our work I not only ensure that the design is clearly written and scoped, we have refinement sessions where every developer in the team can (and frequently do) ask questions so that they understand what the goal is. These sessions are expressly (and publicly) designed to be safe spaces where questions are expected and there are no stupid questions.
loosed-moose@reddit
You're doing junior devs a disservice if you don't pick nits (in good faith)
positivelymonkey@reddit
When was the last time you got a raise for mentoring juniors?
skdeimos@reddit
uh pretty often? if your company does not have a way for your intern's success to feed into your performance reviews, that sounds like a failure somewhere
positivelymonkey@reddit
Oh they do on paper, in reality it's completely meaningless.
loosed-moose@reddit
Yikes, it's part of the job. Were you ever a junior, benefiting from a senior who mentored you? Pay it forward.
SongFromHenesys@reddit
For junior devs - I take my time, usually line by line, I find links to docs to give advice, it can be quite a time-consuming mission sometimes. Worth it if you care about growing them.
For experienced devs - Its usually structured, I know what to expect, but I take my time as well, just much less. Depending on the repo, I know what I'm going to be looking for the most.
For strong experienced devs that I trust - I try to piss the fuck out of them with some nitpicks
liquilife@reddit
Are you the one who wants to pointlessly restructure conditionals that have no negative impact either way?
martinky24@reddit
“If you condition off the negative and exit early you can save a level of indent for the long code block” is unironically one of my favs
solstheman1992@reddit
Exit early is very very good for reducing mental load. It’s god gift to man kind when you are trying to debug shit quickly.
michel_v@reddit
It also massively helps with proper typing.
tofu_ink@reddit
Reducing mental load is the key, i switch between multiple codebases a day. Its the only way I keep my sanity. The jr devs don't understand, but they are learning to keep merge requests small
Creaking_Shelves@reddit
Hey, that ones in the c++ core guidelines so I can drop a link on them for that
yolobastard1337@reddit
you're not alone -- iirc that is the first technique in "tidy first?" by kent beck
positivelymonkey@reddit
I put in unnecessary else's just to trigger them sometimes.
Else's aren't always redundant, they show branching intent and allow new code to be added safely without needing to understand where the branching intent was.
hooahest@reddit
readability is important yo
jambalaya004@reddit
“You should use 4 spaces here.”
WindHawkeye@reddit
I read the commit message and maybe skim it unless it's something I'm actually worried a mistake will cause long term pain in
Plenty-Attitude-7821@reddit
In case of juniors i automatically ask them to redo the PR twice and then i do CR.
levelworm@reddit
I really hate deep code reviews that are out of the scope of the ticket.
For example theoretically it's good to give a restructuring recommendation if the code smells bad. But it's inappropriate if the ticket just says "add a one liner here". If you insist on a clean up before merging, then you need to create a maybe 5 day ticket instead of stuffing it into the current one.
Steinrikur@reddit
That's scope creep and should be ignored.
Personally, if I see a bug/improvement possibility in the surrounding code I preface it with "Offtopic:". It should not be handled in the same PR - just use the comment to create a new issue.
metaphorm@reddit
I'll go pretty deep but I always try to be very clear and label my comments in one of three categories:
blocker: this must be addressed to get merge approval
nit: this is not a necessary change, just a suggestion, usually to try and improve readability
question: this is not a suggestion, this is a question for my own understanding of what the PR is doing
Steinrikur@reddit
I use the 'Changes requested' and 'convert comment to task' for blockers. I should probably start adding 'question:' for when I just want to know what they are doing or why.
Just today we found out that a change from a few months ago broke our system. The PR had an unanswered/ignored 'What will be the impact of this change?' from me.
nobody-important-1@reddit
Bro, if the boss or lead does “nit” it’s implied that it has to be done
metaphorm@reddit
in some work environments that might be true. it doesn't have to be true. much better to have a team that's really comfortable with communicating clearly so people don't make assumptions like that.
blottingbottle@reddit
It's dev-dependent for me. For some devs I know that "nit = "they won't bother responding about it let alone ever consider the suggestion for future CRs"
skdeimos@reddit
For jr devs I go quite deep. I will definitely pull the branch and mess with it locally because I definitely do not trust them to have done all the testing. I will try quite hard to think of edge cases, or how I can break their code, or think of better ways they could have organized the code to make it more maintainable.
diffyqgirl@reddit
How many PRs do you have to review? I get around 10 a day and I would love to give it this level of attention but it just isn't possible.
skdeimos@reddit
Yeah, that's not okay either -- how big are the PRs and what's the balance of jr to sr devs on your team? 10 thorough reviews a day is not realistically sustainable, but not reviewing them thoroughly isn't an option either. You should probably be flagging this to your lead/team.
FlailingDino@reddit
How big are these PR’s? Pulling a juniors branch, messing with it locally, and deeply scrutinizing it does not sound like a 15-20 minute job to me…but maybe I’m just slow.
skdeimos@reddit
I dunno, 40-100 lines in 2-4 files? juniors should be doing small, simple tasks like bugfixes, no? if it's a larger, riskier PR then of course I'll take more time.
Ok-Reflection-9505@reddit
I wish that Jr devs had to check for all usages of whatever they’re modifying before submitting a PR 😭 they always miss one here or there
cjd280@reddit
That’s what the tests we’ve never wrote would be for…
UnrelentingStupidity@reddit
Not checking usage is like entry level mistake not junior tbh
insta@reddit
"I assume the dev did testing enough to make sure it works and I'll let the testing team find anything else that might pop up."
lol
nobody-important-1@reddit
Don’t be that guy with nothing to do and is overly picky
blottingbottle@reddit
What counts as "overly picky?"
icesurfer10@reddit
Many here say that they review the code of a junior differently to those more experienced. My reviews are the same regardless of who the author is.
Firstly, I will always ensure there's an appropriate linter and build process in place so that I don't need to spend time unnecessarily looking for incorrect whitespace or brace placement etc.
I will trust that the author has tested their work, that it runs and that it does what it's supposed to (at least for happy path).
I will only pull down the repo and run it if it's a change that I think warrants it, but honestly many weeks will pass without me doing this.
For the actual review, I will have a first pass/skim to get the high level intention, roughly understand the requirements they're trying to achieve and just generally gain some context to the change. Given we refine these tickets together, it's normally somewhat fresh in my brain anyway. We expect one another to add a reasonable description to give us the context of the change.
Once I've done that, I'll review the code at face value, making sure I understand all of it. Some people say there should be "trust" in a pr process, but then I ask what the point is if you're going to blindly approve anyway.
SteveMacAwesome@reddit
For junior developers it can be helpful to do the review together. That way they can ask questions and, more importantly, you can verify they understand what you mean.
You have seen the 5-level deep nested ternary nightmare before and can see it coming, the junior dev doesn’t and sees a less verbose way of writing if statements. Being there in person (or on discord, slack, teams) lets you go “check out this chaos over here, this is why we don’t do this” without accidentally seeming smug because your junior team member read your comments while having a bad day.
ToThePillory@reddit
Not very deep, I honestly see code review as somewhere to raise the confidence of juniors, I'm not really looking to find fault unless it's really bad.
If the code works, is tidy, and the IDE isn't showing warnings, then it's probably enough for me to give an encouraging code review with some hearty backslapping.
If I see something bad, I'll point it out, but I'm not really going in there looking to fault find.
Fair_Local_588@reddit
I used to get really deep into code reviews and now I just look for very obvious bugs and if I don’t find them, I approve and trust them to test and find any issues. I’ll also look at PRs that introduce new abstractions that are a base for a new chunk of work.
Overall though, I understand that code is temporary just like us and also I’m lazy 🤪
bentreflection@reddit
For juniors I would never assume anything worked at all or that they even did the correct ticket. Same goes for senior recent hires. For those PRs I have to make sure I fully understand the requirements and check things manually in the browser or wherever. I usually look for potential gotchas and edge cases that a junior probably wouldn’t think to guard against. The biggest issues with these PRs are usually doing things unconventionally or reinventing the wheel when we already have code to accomplish the same thing somewhere else.
For seniors that I can trust I usually check that tests are passing and read the ticket to gain an understanding of the requirements and then make sure I don’t see anything obviously wrong or things that go against convention. If something pops into my mind as a potential bug or untested edge case I’ll mention it but I don’t usually go verify everything myself.
Basically with juniors I assume it’s broken and I need to figure out why and with seniors I assume it’s fine and just give it a sanity check.
sdwvit@reddit
I think reviews can be split into 3 things: - is the code ok? code smells, typos, test coverage - does pull request does what was in the ticket? if feature works as intended, etc. - is the architecture correct? would it cause problems in future in bigger context
I believe it is a reviewer’s responsibility to do the first, authors’s responsibility to do the second, and project tech lead to do the third.
CompassionateSkeptic@reddit
I think of 3 broad categories of review in a corporate context (as opposed to open source) — 1. Structural 2. Functional 3. Logical
A structural review is mainly interested in making sure that the contribution meets expectations and conventions beyond analyzers and linters, has workable semantics, useful comments and remarks, and is generally maintainable.
A functional review is all of a structural review, plus it looks at things like performance, I’ll be more inclined to start technical discussions, I’ll be very liberal with curiosities, I will want to make sure the semantics reflect the functionality, and it is generally a good addition to the code base. This is my default.
A logical review is basically all of the first two, plus I’ll have familiarized myself with the task well enough to form my own opinion on how it should be approached and I will essentially do a differential analysis between the two.
—
For a junior, someone onboarding to a code base, or a mentee, I’ll typically do a 2 or a 3 with a focus on identifying things I like and things to learn. But I keep criticisms normalized to a tone for PR feedback.
discord-ian@reddit
I want to answer one question with every code review. How F'ed am I if this code breaks, the person who wrote it is gone, and now I need to fix it. And what needs to change now, so I don't want to die if that happens. Sometimes, that takes a very quick skim. Other times, that takes a detailed review. I will say over 90% of my comments are for better comments documentation or to improve readability. With juniors that need more help, I try to give feedback and guidance before we get to a code review so they look good in front of the team and whatnot.
gomihako_@reddit
I just ask questions rather than saying “this is wrong”, it forces the submitter to think deeper and own the decision making process rather than just blindly copying suggestions
nickisfractured@reddit
Not just for juniors but everyone, I make my team post images of postman or videos of the application working based on the use cases listed in the ticket. I check that the CI is passing, the test coverage is there and then I look at the code. This makes code review faster because if they can prove all requirements are met based on video or images I can focus on the architecture and style of the code itself without worrying if it’s actually working.
parav01d89@reddit
Copy and Paste from a LinkedIn Post:
Unpopular Opinion: Code Reviews Are NOT About Improving Code Quality!
You read that right. In our team, code reviews aren’t for nitpicking “clean code” or indulging in personal preferences. They’re for ensuring business logic is correct and identifying knowledge gaps. Period.
We don’t waste time on subjective discussions about code quality, and we’ve completely removed ego from the process. If someone says something is wrong, they’d better be ready to define what „right“ looks like. We rely on the C4 Model and document our architecture in IcePanel. The micro-architecture at the code level? That’s up to the developer. If it works, we ship it—simple as that. We’re building software solutions, not working on an art project!
If knowledge gaps surface during a review, we log them and close the loop in our weekly knowledge-sharing sessions. Each developer also gets 5 hours a week for self-study. That means we have a combined 8 hours weekly for team-wide learning and improvement. Yes, that’s on the clock. And yes, it’s part of our job!
Here’s the kicker: if you’re using code reviews to boost quality or teach new concepts, you’re doing it wrong. This slows down your dev process and wastes time on individual learning instead of facilitating true team-wide knowledge transfer.
Stop overthinking code reviews. They’re just one part of the bigger picture!
Now please look back at the product and prepare the deployment.
Today we are shippin‘! ... again. 🚀
PuzzleheadedReach797@reddit
Allways going to the deep.
If desing acceptable, its okey for me, but i check clean and maintable code, futher improvments, Validating logics, searching edge cases,
Make sure they covered happy path with unit tests and manual testing
if im not sure then i ask it, like did you check it x y z or give advice after deployment cheks like, chek service x DB cpu level because we use intensivly this new service,
If this desing can be improvment (and we need this improvment) plus needs more work to refactor i create new followup tasks
PuzzleheadedReach797@reddit
Allways going to the deep.
If desing acceptable, its okey for me, but i check clean and maintable code, futher improvments, Validating logics, searching edge cases,
Make sure they covered happy path with unit tests and manual testing
if im not sure then i ask it, like did you check it x y z or give advice after deployment cheks like, chek service x DB cpu level because we use intensivly this new service,
If this desing can be improvment (and we need this improvment) plus needs more work to refactor i create new followup tasks
ryuzaki49@reddit
Lucky you have a dedicated testing team!
OkLettuce338@reddit
Depends on your role. If you’re responsible for 3am pager duty, then it should be what you would commit.
If you aren’t then you should make sure they follow your orgs standards and aren’t committing bugs
ProfBeaker@reddit
Depends on both how big the change is, and where in the code it is.
For changes that aren't largely mechanical (eg, simple rename, IDE-driven refactor) I'll usually look at every line of non-test code. I tend to just give tests a quick read, but I feel like I should probably do more. If it's a piece of code that's central or very subtle, I might pull it down locally and poke around myself.
I generally assume devs do zero testing, unless it's described in the PR notes or is in automated tests. I also don't trust our QA to find much of anything, and at my last three jobs basically didn't have QA at all. So for me there is no "throw it over to QA", just "throw it into prod".
economicwhale@reddit
Go deep every time. Only perfection is enough.
LGTM 👍
Josh1billion@reddit
If I'm the tech lead, very. Otherwise...
Hypothetically speaking, let's say I'm an IC at a big corporation. Let's say I give a code review my best effort imaginable: I pull the branch, test it locally, spend a significant chunk of time scrutinizing the code, have good back-and-forth discussions with the author. I ultimately dedicate hours reviewing this one PR over the course of a couple of days. Will my boss notice that effort? Or, if I just give it a cursory glance to check for glaring issues so that I can get my own work done faster, is my boss more likely to notice that effort?
ezaquarii_com@reddit
Almost exclusively deep into design. Minor things are mostly automated or linted away.
Disclaimer: we're on a pager duty. No way we want to ship stuff calling us for help at 3am.