Hello all. I work on a small team and I’ve been tasked with taking on the bulk of code review for when my senior goes on leave. Although I’ve read a fair bit of code (man am I lucky I learned to code before AI “got good”) I can’t say I’ve done a lot of actual “review”. Does anyone have any tips? I feel responsible if anything breaks after I review it so of course I want to avoid that.
You must log in or register to comment.
Always phrase things in a positive way. No matter how good or thorough your review is, phrasing issues in a positive way and pointing out good things will make discussion about the code so much easier, and also obviously make the human interaction at work nicer.
Also there may be novice people or people who just didn’t do a very good job with their pull requests. Don’t judge. Remember that you are not grading an exam, but reviewing is a cooperative task.
“This is great, this is ok, this is great.” is an amazing tool. People will always focus on the negative, so presenting a negative as a positive, but juxtaposed with even better positives, shifts the scale. It makes people think about it, without being too hard on themselves. I call it the compliment sandwich.
Another tip that has served me well is the tick of approval, followed by the question “With this part, what would you do differently next time?”. It frames it as self improvement feedback.
That being said, I’m not part of a traditional code review process, so this could all be totally misguided advice.
Wish someone had given me this advice when I was younger!
If you don’t understand someone’s code, be brave and leave a comment saying you don’t understand. If you can’t understand it, the next person who has to maintain it won’t either. It’s a strong indication that refactoring should be done.
Also if something breaks, just remember that you can roll back.
Good luck!
I’ve done thousands of reviews, been a senior for the last 4 years, and a tech lead for three of those. If I could distill what I’ve learned into a few tips:
Make sure you understand what you know what they’re doing and why they’re doing it.
If something feels weird, explore it, your gut is a leading indicator of bad code.
Think about how you would know if there was something wrong with the code after its deployed.
Think about how hard it would be to change the code later on.
Those are probably my big ones for general code reviews, there’s more specific ones for v&v and embedded, but I feel like these apply to everything.
Guidelines, best practices… Every company and every developer team has their own way of doing things. Start from there. If nothing is written down, be the one to do that and make it roll.
The comments I’ve read here are great, take them in.
I would add to be very picky with very junior or sloppy code, don’t let that shit pass. For senior stuff just gloss over and check for glaring issues.
Looking for anti patterns and other code smells is a big part of it. One of the most overlooked pieces I see from new code reviewers is missing new code that allows documentation to go stale. Any changes to code that do not update corresponding documentation is by far one of the easiest way to cause confusion for new team members spinning up. Also, when looking at those documentation updates, verifying the code changes fit in those.
Avoid nit-picking. Only comment on stuff that really matters, especially if you have a lot of comments.
Also good luck!
on the fear part, in a healthy team the author still owns the change, the reviewer is a second pair of eyes not an insurance policy. what helped my people a lot was asking the author how they tested it and what happens if it goes wrong, if they can’t answer that’s your review comment right there. and if something breaks after you approved it, you fix it together and add a check so it doesn’t happen again, nobody serious blames the reviewer for that
Review to understand, not necessarily to find problems. If you do find bugs and anti-patterns along the way, great, ask about them. But consider it your goal to learn what they did, why they did it this way.
When you’re giving feedback, phrase things as questions most of the time. Idk about you, but I’m rarely so sure of myself. If you’re saying stuff like “Would it work better if we tried xyz?” instead of “Change this to do xyz.”, people respond better. Sometimes it starts a discussion, sometimes you learn they had a good reason for what they did in the first place.
It’ll be really tempting to skim and just give it a quick Looks Good To Me. Try not to.
Nitpick copyright dates and white space.
Be sure to be extra shitty about it.
Fortunately we use formatters so I don’t have to be too Ruff with the white space lol
I think this guide is pretty good: https://google.github.io/eng-practices/review/reviewer/
I’ll give it a read, thanks.
This is a little less about reviewing and more code standards but I’m a firm believer in functions only doing one thing. I’ve caught so many bugs by enforcing that during my code reviews. Determining “only one thing” is kind of by experience where you should draw that line but I find if a function is pushing 25 lines of code or more or has more than one indent it’s suspicious.
It’s more work to refactor and keep things clean enforcing this, but boy has it made my life better. It also makes the code ultimately easier to review as you have to keep less in your head at once for context too.
You’re not going to be good at it, at first. It probably takes a good 6-12 months to do it all correctly and most developers don’t actually learn how to do it all properly.
But here’s my cheatsheet if you actually want to spend the time to get really good at code review. Save this comment so you can keep coming back to it.
First, you need to relearn the programming side, making code in a specific way that has the capacity to be reviewed correctly. Don’t think about code made to accomplish a goal because you should already be able to do that. Instead make sure you understand what makes code easy to review by someone else.
Why: You may have been stuck in an environment where someone else was either fixing your mistakes for you (depriving you of your opportunity to get better at writing reviewable code), or you may have been in an environment where you were getting the wrong feedback.
How: Read this link: https://mtlynch.io/code-review-love, then read it again next week, and again the following week. Once you intuitively can make reviewable code, it’ll be easier for you to teach others how to make reviewable code (which makes your reviews easier to perform correctly).
Second: Learn how to actually give feedback in a way that gets things fixed.
How: Read this: https://mtlynch.io/human-code-reviews-1/.
Third: Don’t try to write or correct documentation before you understand the “Grand unified theory of documentation” Link: https://docs.divio.com/documentation-system/. TL;DR: Most programmers don’t understand that there’s actually four different types of documentation and assume that the kind that they right is what’s actually needed.
That goes into technical writing, which is another skill you need to develop but makes projects better as a whole, which makes it easier to review code.
Fourth (bonus reading material): antirez (the guy who made redis) went through his whole project and did a kind of census/retrospective on all the comments inside. It sort of accidentally turned into a good practical advisory on writing comments that actually mean something. Link here: https://antirez.com/news/124
It’s a broad topic.
Be mindful of how you communicate. Differentiate between questions, risks, wrongs, subjective. Talk with your teammates. Establish a common understanding of deliberate word use or forms, intentions, and necessities to answer or change or not.
Consider whether passive and non-aggressive communication is the best approach. Some people may not pick up on defensive tone and just not care. Maybe they need more direct and pushy speech and discussions where you point stuff out, including the overall approach to working together and your assumptions or expectations.
Be mindful of sharing alternative approaches or knowledge vs not commenting to reduce own time investment and number of comments. Discuss with your team where the sweet spot is. This may change over time and is different with different people, their experience in general and in the project.
Make it clear that the one submitting the code is the owner and person responsible. They have to take ownership.
If necessary, establish a common understanding of where to commit time investment. Usually it’s much preferable for the author to invest time into a well documented submission over a reviewer trying to understand and find things out through analysis and questions.
Try to find a common understanding of not just the solution but what the authors understanding and intentions, what edges and alternatives did they consider, what risks or decision basis did they use.
Establish understanding on what your role as reviewer is. Risk reduction? Common standardized solutions and code? Are you lead and give long term direction, including how things are implemented to reduce long term complexity and variance? Do you teach through reviews?
If necessary or unclear, ask what or how deep you are being asked to review. Depending on the author expertise and trustworthyness and your load, sometimes merely approving a concept and a broad peek into the most risky parts or what you want to see the approach on may be enough. Sometimes they may be confident and just want approval on the concept, not every code change.
Be mindful of when to not post comments but ask for a shared screen meeting and talk for them to walk you through or explain things. The author should also be mindful of when they think discussion before asynchronous written review makes sense.
Consider whether you are always the one who should review. At all, or first. Maybe someone else can. Maybe you can do a second review pass, maybe only on remaining open questions.
My thought process always starts with: There are bugs / bad patterns / typos / non-standard blocks / whatever here, I just need to find them.
That’s better than thinking, “there’s probably nothing wrong with this” / “looks good to me”. If you start from the place of knowing something is wrong, you’re more likely to find something, IMHO.
