Programming

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.

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

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.

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.

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.

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.

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!

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/

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.

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.

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.

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.