Any good resources you can recommend to improve my own code review skills? I'm keen on drilling into the most valuable skills needed in the industry right now. It sounds like good code review is super important but lacking.
It's really difficult to actually do a thorough check of logic , exploration of alternate approaches, performance concerns, etc. in all your code reviews, it would be an insane amount of time spent and probably no real return on investment for the company (obviously devs would love to spend a couple hours weighing the merits of different approaches to a problem).
I have a list of checks to run through, some general (Sass variables used, reusable code/components used if possible, variables have meaningful names), others more specific to our codebase conventions. I add to the list as new conventions get prioritized and check it. Code review is more about maintaining codebase consistency than suggesting alternate solutions to problems for me.
Awesome. Thank you for the distillation. I found an insane amount of info as I started searching, so your breakdown of what you consider during reviews helps.
Measure and adjust: Find a source of high-quality code reviews; the easiest source may be the high-level engineers at your employer, but if that's not possible/helpful, I'm sure there's free/open sources. Make your own code review before reading others, then compare and contrast. What did they flag that you didn't notice? Can you generalize to categories you could prioritize more? What did you flag that they didn't? When you both flagged the same thing, whose phrasing was more helpful?
More personally, here's my approach, which may or may not be helpful for others:
* First, skim the entire PR. On a raw, visual level, get a sense of what it looks like. A couple big edits, or a bunch of small updates (like changing a variable/method name, changing an imported library, re-ordering some interaction, etc)? A bunch of indentation changes without much else? An extension of an interface? This is to familiarize myself with the general question of Just What's Going On Here, basic context to use as the foundation for the real review. This should take just a minute or two.
* Second, perform a close reading of the diff. Don't worry about high-level conflicts like ordering of operations or other semantic considerations; stick to syntactical-level concerns and context-free mistakes. Are the variables and other symbols well-named? Is exception handling done properly, is this library not initialized correctly? At the "fit and finish"/polishing phase, this might be the majority of the effort. At the early-draft stage, it may be counter-productive to actually note all the possible issues. Either way, to use an analogy to compiling source code, the goal should be to build an AST of the diff, so you can begin to build a model to manipulate through the rest of the review.
* Lastly, address the higher-level qualities. Depending on whether you prefer a bottom-up or top-down approach to design, you might order this section differently. I tend to try to identify where on the abstraction scale the highest-leverage feedback resides, and then focus on that level. Depending on context, that could mean discussing which component needs to change, which repository/module this change should live in, which classes should be changed/created, what's the behavior of specific methods, etc.
Whatever your feedback, consider what you hope to accomplish with the comment. Are you trying to educate someone about an idiom they don't know about? Ensuring correctness of a complicated interaction? Maintaining a common code style or test coverage threshold? Make maintenance/debugging simpler? Is this something that you view as blocking, or is it more of an FYI? If the latter, it's pretty important to communicate that clearly.
