PR Review Guidelines: What I Look For in Code Reviews
These are notes from my personal checklist of things I look for while reviewing pull requests. I picked these up from comments I received on my own PRs from seniors, patterns I observed in other reviews, and lessons learned from production incidents that made me think “we should have caught this earlier.”
This is not an exhaustive list, and I would not present it as strict doctrine. It’s a general guideline I refer to. Obviously, there are cases when we trade off thorough reviews for POCs or hotfixes. Maybe this can help both code authors and reviewers as a checklist.
I do not intend to favor a particular paradigm (functional vs OOP), language, or framework. My objective is to summarize a generic guideline that works across most use cases.
1. Write in the Natural Style of the Language You Are Using
Every language has its own idioms and patterns i.e. a natural way of doing things. When you fight against these patterns by borrowing approaches from other languages or ecosystems, the code often ends up more verbose, harder to maintain, and sometimes less efficient. When the code looks “native” to the language, tools like compilers, runtimes, and linters (Clippy) can recognize standard patterns more reliably. Compilers apply optimizations when they can prove certain properties about the code, idiomatic patterns make these properties more obvious. Taking a few examples from Rust below.
Rust: Prefer Iterators Over Manual Loops
Rust prefers iterators with collect(), map, and filter chains over manual loops with indexing when possible.
Why iterators can be faster:
- Bounds check elimination: With manual indexing (
items[i]), Rust must verifyi < items.len()on every access. Iterators eliminate these runtime checks because the compiler knows they won’t produce out-of-bounds indices. - Potential for vectorization: Simple iterator chains like
mapandfiltercan make it easier for LLVM to recognize vectorizable patterns and apply SIMD optimizations. That said, LLVM can also vectorize simple loops, and this isn’t a guaranteed win, but iterators don’t hurt and often help. - Zero-cost abstraction: In practice, simple iterator chains often compile to identical or better assembly than hand-rolled loops. Refer these for detailed analysis How to avoid bounds checks in Rust and The Humble For Loop in Rust
Manual loop with indexing:
let mut result = Vec::new();
let mut i = 0;
while i < items.len() {
if items[i] > 10 {
result.push(items[i] * 2);
}
i += 1;
}