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.

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

Nitpick copyright dates and white space.

Be sure to be extra shitty about it.

It’s not your job to find bugs. If you do that’s great, but the author should have done that. It’s your job to ensure the code is well structured, understandable, and follows team norms.

Ask questions if you’re unsure of why something was done the way it was. Sometimes even just asking “why’d you do this” will spark a great design conversation. And if something is confusing it’s worth exploring more clear ways to implement it.

This sounds like a perfect moment to level set expectations with your senior, clear up anything that’s hazy, and establish a set of required steps while performing code reviews.

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.

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

Well for starters, having one person be the reviewer is not a great idea. One reason is that you have a single point of failure, as you are now discovering.
Another reason is that it creates a codebase that reinforces one persons biases.
Thirdly it can (potentially) silo knowledge, so if you only ever work on the data layer, you have no insight into any other part of the codebase.

I would encourage you to try get others in on the reviewing effort. I’d prefer assigning people to review, rather than just hoping someone steps up. (There is a saying; If it is everyone’s job, it is nobody’s job)

The actual review is multi part, Step 1: understand what was supposed to happen. (Was this change supposed to update a dependency? Update some text? Add a button?).
Step 2: confirm the code builds. (I ain’t reviewing shit if it doesn’t even compile).
Step 3: reading the code. You’re looking out for a bunch of different stuff; access control, function size, file size, design patterns, cyclomatic complexity, weird names, weird types, overly broad implementations, complexity where simplicity should be, threading shenanigans, lots of comments (usually bad). The list is nearly endless, but trust your gut, if something feels off that’s called a Code Smell, and it warrants further digging.

It is very ok to reach out to the author and ask why choices were made, if they don’t have a firm answer, it’s time to get concerned.

Once the code looks good, I move on to tests. Check that they build and pass. Then start looking at what they actually do.
If a function takes an input, I like to add 3 kinds of test: Expected good inputs (like valid passwords, or correctly formatted date strings) Expected bad inputs (zeros and empty strings, malformed inputs) and lastly: Fucking chaos (+-int_max, the first chapter of Moby Dick, actual face rolling on the keyboard) a function with one test is not tested at all.

Finally, at long last, actually run the code. Verify the code does what it set out to do.

PR Approved.

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!

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.

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.

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.

Avoid nit-picking. Only comment on stuff that really matters, especially if you have a lot of comments.

Also good luck!

I think this guide is pretty good: https://google.github.io/eng-practices/review/reviewer/

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.