How deep do you go in code reviews?

[-]

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

[-]

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.

[-]

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.

[-]

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 🤪

[-]

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

[-]

nobody-important-1@reddit

Bro, if the boss or lead does “nit” it’s implied that it has to be done

[-]

nobody-important-1@reddit

Don’t be that guy with nothing to do and is overly picky

[-]

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.

[-]

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

[-]

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?

[-]

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!

[-]

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.

[-]

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.