How do you review a PR when parts of it are outside your knowledge?
Posted by Worried_Lab0@reddit | ExperiencedDevs | View on Reddit | 59 comments
I am curious how others handle this.
When you review a pull request and you come across concepts, patterns or parts of the code that you do not fully understand, what do you do next?
Do you take time to investigate that topic and try to understand it on your own, or do you ask the author directly?
How deep do you usually go before approving or requesting changes?
I would love to hear how more experienced engineers approach this.
southp0105@reddit
There are already plenty of good answers to this so I won't reiterate on those.
I'm curious about what's the context behind this question though. Is there any struggle you are experiencing now that made you wonder if your way of reviewing code is wrong?
Worried_Lab0@reddit (OP)
I feel that on some PR’s I have to use a lot AI to explain the code. Which I am doing it for learning but I feel bad when AI catches something that I didn’t and then I will have to leave a comment regarding the issue.
southp0105@reddit
Thanks for the additional context. That's super helpful.
I'm reading two potential factors from the feeling bad:
Before diving into these two feelings, I'd like to step back a bit and share my side of story. Whenever I do code review, I ask 4 questions:
Answering the first 2 Qs normally requires collaboration with the author. If I understand the purpose perfectly and it can't be summarized concisely, the code likely needs to be decomposed into several more isolated units. If I don't know how to test it, I'll collaborate with the author to improve my product knowledge, and the author will also likely need to improve their documented test plan.
For the 3rd, I do dig into the codebase and study all the relevant materials to make sure that I fully understand it. I'll time-box it though; if I can't do it within a practical time, I'll just ask, and be honest that I don't understand it and solicit other team members' opinions. Is it time-consuming? Yes. However, accumulating this seemingly small but diligent effort is actually the key investment that guarantees you to develop deep understanding to the product you are working on. The next time you see the same thing, you will just snap it. That deep understanding is the only way to unlock the Q4: can we do better. Guess what? Often time I can suggest improvement even for the code written by someone much more senior by doing this way.
You can see how AI can make this whole process a lot easier — I'm benefited from it a lot already. It helps me bootstrap my knowledge to the area that I'm not so familiar with, and gather relevant information for me so I don't have to go surfing a million sites.
Now, going back to where we were originally at: first of all, please don't feel bad at using AI. It's just a tool. Like how they like to say, you are absolutely right to use it. What I'd recommend is to use it more like an assistant in building deep understanding more efficiently, not just performing the review.
It catches something you didn't? Great. Learn them deeply. It suggests fixes that you didn't think of? Awesome. Make sure you perfectly understand them as well. And then ask, can we do better? With this level of effort, you shouldn't feel bad at leaving a review based off AI's findings. Think this way: how would you report an issue caught by a static code analyzer? Just do it the same. AI is an assisting tool; we don't have to attach a character to it no matter how high their resemblance is. Also, you will likely start noticing a good portion of their opinions are not true.
By doing so, the deep understanding you accumulate over time will resolve the incompetency and unauthentic feeling that's bothering you.
Sorry that my reply is 3 days late. Hopefully I can still help you resolve some anxiety around this topic. If there is anything I can help further, please feel free to let me know.
Tiny_Ad1105@reddit
Ask them to explain the bits I don't understand. PRs are a sharing mechanism. The reviewer should take it as an opportunity to in the gaps in their knowledge. And if the author can't explain their own code easily, then that's a good sign to revisit it
delventhalz@reddit
Most of my review comments are questions. Sometimes it’s just a good non-confrontational way to present something, but other times I legitimately don’t know and just want to make sure they’ve thought about it.
AllTheWorldIsAPuzzle@reddit
Our department has been gutted. So my code goes to another developer who doesn't understand things like regular expressions, then gets passed to our boss who was a business major and he signs off on it.
Harlemdartagnan@reddit
classic
Krom2040@reddit
In my opinion, code reviews are entirely about code design and patterns, not about correctness. Occasionally you’ll identify a problem with the code, but that’s essentially just a happy accident. There’s absolutely no way that developers could reliably spot bugs when reviewing substantial amounts of code, and that’s really what your integration and unit tests are for anyway.
stubbornKratos@reddit
As a reviewer it’s also your responsibility to ensure you have appropriate testing no?
I feel like part of my whole role as a reviewer is to ensure that the changes come along with suitable testing and documentation to affirm its correctness.
Which isn’t to say that bugs can’t and don’t slip by, but if I don’t have confidence in the correctness of a change either it needs to be covered with testing or have documented behaviour
Krom2040@reddit
It’s a fair point, and I’ll admit that I tend to just skim the unit tests for basic existence unless I see something really complicated. I have a feeling I’m not the only one guilty of this, though!
ice_dagger@reddit
reviewed-under: stuff/i/understand
Harotsa@reddit
lgtm
dnszero@reddit
Fixed it for you: lgtm?
Frozboz@reddit
100%. I'm on a team full of seniors so it's pointless to waste chunks of my day spending more than minimal time reviewing code from developers that know better.
local-person-nc@reddit
Exactly. Al these devs talking about jumping on calls, have to 100% approve every line of code, yada yada yada. My job is to look at the code and make sure it's sound. Not do a ful QA like some ego gatekeeper of all code. Bunch fucking losers.
Fire_Lord_Zukko@reddit
Why is it ego to gatekeep bad code? I work on a small team with fairly small line of business apps.
theonlyname4me@reddit
For me it is all about risk.
If it is a risky change I’ll get my hands real dirty and pester the author/my peers; if it’s some internal tool that 7 people use 🤷♂️.
Being a good engineer is about being able to know the difference and make the trade offs.
Ibuprofen-Headgear@reddit
Make sure it’s not malicious or doing anything glaringly wrong or illegal, then yolo
tehfrod@reddit
You review what you can and comment "FYI I'm not reviewing this for [domain] because I don't feel knowledgeable enough to do so."
niftydoesit@reddit
Don't be afraid to be the idiot in the room is a mantra I go by.
I always ask the author the question if I don't understand a part of the code and I am confident that someone else is thinking the same. In addition, with all this AI generated stuff going about, it forces authors to think about the code they are putting their name to if they weren't even the ones who wrote it in the first place.
bentreflection@reddit
Read the ticket so you're familiar with the issue being solved.
Ask the developer to walk you through the high level flow of how the feature functions
Ensure the tests are clearly testing and asserting that the ticket requirements are being met. Try to think of edge cases that are not being tested
Look through the code and make sure everything is following conventions
When you find chunks of confusing or non standard code ask the author to walk you through it. If A. they can explain it, B. the explanation makes sense to you, and C. you can't think of a better way to make it more clear, then ask the author to write a code comment detailing what they just explained to you.
After all of that is done the code is likely good to approve assuming there was no one better to be doing the code review in the first place.
Personally, if it's something i think would be good for my career to know more about I dive in deeply. If it's just some random one-off thing I don't spend my time going too deeply on someone else's PR.
nsxwolf@reddit
This is what Bikeshedding was made for. Find a spelling mistake or something.
bluetista1988@reddit
"I don't think this variable name is descriptive enough, please reconsider" meanwhile you have a deadlock issue sitting three lines down.
Basic-Kale3169@reddit
This is where LLMs shine. It's an amazing learning tool and it's perfect to use when doing code review.
You can go WILD.
Use a few prompts to understand the intent behind the PR, but also to gain domain expertise around it.
Use a few prompts to see if there could have been different approaches.
Use prompts to see if there are any potential risks, performance regressions ,etc.
CymruSober@reddit
You’re essentially using the LLM to launch a DoS attack on your teammates
Basic-Kale3169@reddit
Did I tell anyone to copy paste whatever the LLM is giving you in the PR?
Treat code review as a learning experience and you will go far.
Worried_Lab0@reddit (OP)
I always feel bad to do that because I feel that I am using the LLM against them with things that I would never thought about.
bwainfweeze@reddit
Ask questions.
PRs are where new idioms show up for the first time. And your best coworkers will realize that questions also count as feedback. They may not know that part of their solution was surprising until someone says boo.
SnugglyCoderGuy@reddit
I will investigate and ask questions, a lot probably. These are prime times to learn new things and you will look at sonething with a fresh perspective which can iften bring new insights and catch errors.
Upbeat-Conquest-654@reddit
I usually ask the developer who made the changes to walk me through the code. How does it work? What does this part do? Why did you do x? Why didn't you do y?
Frozboz@reddit
That's too much. Code reviews are for checking patterns and design, both of which you can check without knowing all the details as to what the code does. We aren't reviewing for correctness.
Frozboz@reddit
Depends on the developer. If it's someone I trust, I rubber stamp it and move on. If not, then I'll ask for some input or context.
bazeloth@reddit
When I review I tend not to review for correctness unless it's a very small PR. I tend to review the process and the related ticket and whether it'd solve the problem. The OP has the domain knowledge and especially with proper test coverage I simply do sanity checks.
Codebases tend to get big so unless I have first hand experience the colleague who made the PR rightfully assumes I don't know the subject.
eyes-are-fading-blue@reddit
So you don’t review the code?
bazeloth@reddit
Where did you get idea?
eyes-are-fading-blue@reddit
That you do not review for the correctness and instead you review for “the process and the related tickets”.
Not gonna lie, sounds absurd but each to their own.
bazeloth@reddit
Yes the seperate commits and see if they logically follow each other to solve the problem described in the ticket. I'm not overseeing 20 changed files and how the changed code correlates to the existing codebase unless I have worked with that code before.
You can't expect reviewers to verify whether every piece of code compiles, is tested, doesn't introduce breaking changes and/or fits the coding guidelines. That's up to the IDE's compiler, test coverage reports, linters and possibly a code owners file. I will ask whether test coverage is present but I won't checkout your branch and test your changes.
Mike312@reddit
Depends on what it is I don't understand, but if we're talking specifically about code, it should be broken down into functions and variables that are named with appropriate verbs and nouns. Like, first step. If that hasn't happened, then that's an issue.
I should never see MoreData() with variables a, b, and c, I should see GetCustomersByState() with geo_state, filters, and result. Or Customers.GetByState(), depending on your system design/architecture.
R2_SWE2@reddit
lgtm
chikamakaleyley@reddit
I've been doing a lot of this since I've been at my current company for about 3 months
If I'm really really new to the codebase I just try to look for things that don't quite line up logically - in this case I'm just really asking questions (added as comments to the review) as I try to familiarize myself with the codebase in general. In most cases I usually give the approval if nothing is glaring with regards to logic
As I get more familiar with the service and from reviews of my own PRs, then I try to make more sense of the PR changes based on my understanding. I go back to the author and describe to them how I understand whats going on, they confirm or deny, and it helps me understand the bigger picture, approve as needed
It just takes time and like curiosity of whats in your codebase
InvestmentGrift@reddit
glance it over. you know how to review basic stuff, like, is there a thousand nested loops, or if blocks, are variables named bullshit like ix jx, b, a, abc, c, are there giant amounts of repeated code that are begging to be consolidated into a helper, etc. other then that go ahead & assume they know wtf their code is doing
Whitchorence@reddit
The review is a favor to the author so they should be willing to answer questions you have.
gemengelage@reddit
PRs are just as much a tool to share knowledge and responsibility as they are to assure quality. Sadly a lot of people focus too much on the quality assurance part and treat it as yet another checkbox.
What I do (and what I also train my juniors to do) is ask questions. I'm usually the second reviewer, so I sometimes take my comments and contact the first reviewer and ask them what they think about that and often the answer is something like "I didn't know what that is, so I couldn't correct it" and they didn't want to look stupid.
I wouldn't expect this to work out well at all workplaces and for everyone because, let's be real here, if you're not great at your job and your project has a hostile culture, you might actually look stupid to the people in your project.
But I also don't think I have to explain how sharing knowledge and not being afraid to ask questions is the better and healthier path for every project.
se-podcast@reddit
Answer: You do not. Review the parts you know. Tap others on the shoulder to cover areas outside your domain.
This episode of Social Engineering might be relevant for you: https://socialengineering.fm/episodes/how-to-have-a-good-pr/
fake-software-eng@reddit
Item
Budget-Length2666@reddit
You wait at least 1 minute. Then you write a comment saying "LGTM", then you approve. Then you move on to something else.
Abject_Parsley_4525@reddit
I've only ever really encountered this once or twice, but when I review a PR I really get stuck in there, I run the code locally, and I'll hop on a call with the author if I spot anything off the walls. If it's way, way out of my depth, I might re-assign it but like I say I have only had to bow out a handful of times due to timing / relevant product area knowledge.
i_exaggerated@reddit
Review the parts I can contribute to, admit I’m not good to review the parts I’m not, suggest an alternative reviewer for those parts, pay attention to their review so I can learn more.
Sometimes it takes a village. This way we all get more familiar with the codebase.
deux3xmachina@reddit
To add onto this: If the team's not large enough, or the only person with appropriate domain knowledge is the person askisg for review, I've had good experiences just asking for clarification. I might not know enough to point out issues, but I can at least ask things like "hey, we seem to be doing X frequently, is there a reason we haven't put it in its own function instead of copy/pasting?" or "this looks like it might have poor performance, is there something I'm missing or would it be possible to rewrite in another way?".
Basically becoming a rubber duck for the areas I lack expertise, which gives the author both the opportunity to reduce the bus factor by sharing knowledge AND potentially catching issues by having them re-analyze their solution with a different mindset.
It's not perfect, but it's helped catch a few things that can be improved and almost always helps the team grow by explaining why certain things were/weren't done.
Altruistic-Cattle761@reddit
I don't. If there's something I don't understand, and cannot or should not become competent in it, I say, You need to find another reviewer.
Generally speaking this situation should never arise? I guess I don't understand the question. If your review is required it should be on code that you or your team owns, and you should be sufficiently conversant in all of that.
codescapes@reddit
If you want to actually learn the area of the code then attempt to read and comprehend it by yourself for at least 20 minutes before reaching out to the author.
The kneejerk response is to go "ahh I don't understand I should ask" but if you just sit with the discomfort for a little bit, read the ticket, properly read the code changes etc you pick up way more than if you get a guided walkthrough.
Also when people walk you through code they're telling you what they think they've written, not what they've actually written. Big difference if you actually want to review things properly.
Tbh true PRs on any non-trivial code are not a quick thing and can reasonable take 1hr+. Most teams do incredibly superficial "reviews" that are basically just rubber stamping. "LGTM".
teerre@reddit
It depends what it is. If it's an algorithm and there's literature, I go there. If it's some sort of visual widget and I can check the behavior myself, I do that. If it's business logic, I talk to the person etc.
magichronx@reddit
Not being able to understand a PR is a pretty big code-smell.
I'd definitely have the author explain it to me thoroughly before I'd sign off on it, and ideally request solid PR description/notes for future reference. Imagine how much worse it'll be if you get tasked with maintaining some code/feature you don't understand
Responsible_Boat8860@reddit
It depends on how much I trust the developer. If they have a history of good PR's, then I would only look at code issues rather than intent. If they're someone who needs more scrutiny, I'm running their code locally to make sure it works - even though I don't fully understand the domain. Sometimes, I'm like - Let the QA's handle it...
sc4kilik@reddit
You guys actually review the code?
Dziadzios@reddit
Ask about it. Reviews are a learning experience, to keep knowledge from being siloed. Whoever wrote it, should be able to explain it. You're there just for sanity check.
eyes-are-fading-blue@reddit
There are three layers to code base I work with.
1) Parts that I own/supervise 2) Parts that I jointly develop 3) Parts that I rarely touch.
I only perform basic sanity/style check to 3. 1 and 2, I take my time to understand the code and review thoroughly.
serial_crusher@reddit
Most work is iterative improvements and bug fixes on top of existing software. It should in general follow established patterns. If you’re new to the company/team and are just like “hey, I haven’t done much with this part of the product yet. Can you walk me through it?”, just talk to the person who wrote the PR and say exactly that.
When somebody’s introducing new design patterns, new frameworks, etc, there should be a larger discussion with the team about the plan before any of it gets implemented. If it’s reached the PR level and that discussion hasn’t happened, start asking questions about why the person made the decision they made, and try to pull in other people who have expertise in that area so they can see if it is being done right. At your next sprint retro or whatever, bring up the need to plan those things more in advance, and try to get alignment on that.
_Atomfinger_@reddit
I get on a call with the author, or meet up physically and have them explain it to me.
Deep enough that I'm confident the changes are suitable for production.