Doing code reviews is mostly about spending thoughtful time reading code. Sometimes people focus on what things should look for during a review, and there are plenty of suggested checklists out there, or you can look at other code reviews among your team and make a checklist by paying attention to the patterns. This one habit will get you through most normal code reviews.
I think it’s easy to review code of someone who is new to a language. They may not write idiomatic code, or they may misuse certain language features as they learn a new language. If you know more than them about the language, the review is easy.
It’s also mostly easy to review the structure of the code for logical arrangement — for example, are objects and data models and abstraction used in reasonable ways? Are there reasonable interfaces? Is there reasonable defensive programming and error handling? If you are thoughtful about each of these areas, the review is easy.
Whether or not the code changes as a result of the comment is not what make a comment valuable.
It’s even easy to review code for functionality…check it out and attempt to run it.
Are some engineers better at each of these things than others? Yes. Some people are experts in languages. Some people are experts in modeling. Some people are experts at writing automated tests. Different people are good at different things. Some people are good at lots of things.
Every engineer is composed of different strengths, and for ease of discussion, let’s assume there are 100 dimensions to being a good engineer. Most good engineers are probably strong in about 70 of the traits, and part of the value of a balanced team is that everyone has a different 70, and once you get a certain number of people on a team, the team has strengths in all 100 dimensions.
But what happens when you’re a normal, even above average engineer, and you’ve got to review the code of one engineer who singlehandedly has at least 95 traits out of the 100? What can you possibly tell someone who is just objectively a better engineer? Why even bother? They’re probably right anyways…
Any really good engineer knows they’re good. So do the people around them. This isn’t about ego, there’s usually just a gentle acknowledgement. A really good engineer is obvious to everyone. The unexpected side effect is that sometimes this results in the really good engineer not getting feedback on their work, because everyone has the same impression — they read the code and go “hey this passes my checklist, but of course it would…approved!”.
This is unfortunate, because I think virtually everyone who is good at something enjoys it when someone takes time to engage with their work and provide commentary, good, bad, or neutral. It’s not as much fun to get rubber stamp approvals. No one is accidentally a good engineer — if they consistently produce quality, it’s because they’re putting thought into their work.
As a result, I try to leave some type of comment every time I read a PR. Sometimes I’m reviewing code where the author has more domain experience than I do, but I’ve found some techniques that help.
One of my best techniques is that as I read through the code, I leave comments like “So in this method, you’re basically….” or “Is this because….” Essentially, I just write down observations and questions. There’s no attempt to “give feedback” per se, it’s more like I’m validating that I was able to understand their code.
This is a crucial step.
There are lots of things you can have an opinion on, and it’s better to post something than nothing. Even a “simple” observation is something that even the most junior member of the team can post.
Whether or not the code changes as a result of the comment is not what make a comment valuable. The goal should not be to only post comments about suggested changes. The goal is to have a discussion about the code. Posting a simple observation, almost “restating” their implementation in a sentence or two, can be a valuable thing.
When someone gets comments on their work, it makes the author feel good that someone’s paying attention, and it also adds an important validation step. By explaining your interpretation of the code, you’re validating that it can be maintained in the future. You’re also contributing to documentation. Years in the future when someone is trying to understand something, and they jump back to the PR, they see the discussion and can validate their own understanding. This “verbal confirmation” can be deeply valuable to you and to other people on your team.
Put another way, code written by a senior engineer shouldn’t only make sense to other senior engineers. Part of what makes a great senior engineer is that they produce solutions that are maintainable, which means that even a more junior engineer should be able to understand it.
I think this is really the core spirit of “how to give feedback to an engineer who you know is better than you”. Even if you don’t have a “critique” of something, you can add value by doing nothing more than add comments that just explain what the code is doing, or that explain your interpretation of it.
Before you start the review, think about the functionality and come up with a 60 second guess as to how you’d write the code. This will give you a starting point. You’ll have a perspective (even if it might be incomplete) and then you’ll find it easier to add comments like “why did/didn’t you do it this other way?”. This comment pattern is a gold mine for fostering a good discussion.
It’s worth your time to do reviews. At minimum, you become a better engineer by reading code. If you know a piece of code was written by someone whose skill you admire, why not take the opportunity to study it? And if you study it, why not write down what you take away from it? This is what starts a discussion, and this is where learning happens. Plus, it helps the author. It’s just good all around.
If nothing else, add comments of things you thought the author did well. If this engineer is as good as you say, compliment what you think they did well. Maybe they’ll even respond with some more “yeah I thought this would be a good idea because X” and they’ll even have some other reason you didn’t even think about, and then you learn even more. Again, the goal is to have a discussion about the code.
My overall point is that there’s basically always something you can write on a PR, no matter what kind of skill difference there is between the author and reviewer. The point of code reviews is not just finding bugs, or fixing problems. There’s always some kind of discussion that you can have to break down silos, improve understanding, and improve your individual skills as well.