The short version: review for correctness under volume, for security, and for whether the thing matches what was asked. Leave style to a linter and leave personal preference alone entirely. A Salesforce review that is worth the hour catches the query inside a loop, the missing sharing declaration and the flow that will fail silently on record 51. A review that is worth nothing renames variables and argues about whether the brace goes on its own line.
Look at these first
Volume behaviour. Every SOQL query and every DML statement, and whether it is inside a loop. Then the less obvious version: a method called once per record that itself queries. Ask what happens at 200 records, because that is what the platform will hand it.
Sharing and field access. Does the class declare with sharing, and is that the right choice? Are field and object permissions checked where user data is being exposed? Is anything being written from a context that runs as system? This is the category where a bug is a data breach rather than an error message.
The requirement. Open the ticket. A technically clean implementation of the wrong thing is the most expensive outcome in a review, and it happens constantly because reviewers only read the diff.
Failure paths. What happens when the callout times out, the record is locked, the batch job fails on chunk seven. Silent failure in asynchronous Apex is the defect most likely to reach production unnoticed.
Tests that assert something. Not the coverage number. Does the test check the outcome, does it cover the bulk case, does it cover at least one negative case, and does it create its own data rather than relying on what is in the org.
Reviewing Flow, which is harder
You cannot read a flow diff. The XML is unreadable and the visual editor is the only sensible view, so open the flow in the sandbox and look at it properly. What to check:
- Anything inside a loop. A Get Records, an Update, a callout via Apex action. This is the flow equivalent of a query in a loop and it is the most common serious defect.
- Fault paths. Every element that can fail should have one, or the flow should fail loudly on purpose. A flow with no fault handling fails in a way the user cannot interpret.
- Entry conditions. Does it re-trigger itself? Is the “only when a record is updated to meet the condition” option set where it should be?
- Before-save versus after-save. Field updates on the triggering record belong before save. A lot of avoidable recursion and a lot of avoidable time comes from getting this wrong.
Leave these alone
- Formatting. Automate it or accept it. Arguing about it in a review is how teams learn to dread reviews.
- Naming, unless it is misleading.
acctListversusaccountsdoes not matter. A variable calledtotalthat holds a count does. - Approaches you would have taken differently but which work. If it is correct, tested and readable, your preference is not a finding.
- Pre-existing code the change did not touch. Note it separately as a ticket. Expanding scope in a review is how a two-day change becomes a fortnight.
How to run the review itself
Keep changes small. A 40-file pull request gets approved without being read. Nobody admits this and everybody does it. Under 400 lines of real change is where review quality holds up.
Label the severity of each comment. Blocking, worth fixing, or just a thought. Reviewers who do not do this leave authors guessing, and authors who guess either ignore real problems or spend two days on opinions.
Ask rather than instruct when you are unsure. “What happens here if the list is empty?” is better than “this will fail on an empty list”, because half the time the author has an answer and you have learned something about the system.
Review within a day. A review sitting for a week costs more in lost context than the review saves. If you cannot do it, say so and pass it on.
Frequently asked
Should reviews be mandatory? For anything reaching production, yes. The exception worth allowing is a genuine incident fix, reviewed retrospectively the next morning.
Can a static analysis tool replace this? No, but it should handle the mechanical layer. Run one in the pipeline so humans never comment on a query in a loop that a tool would have caught.
Who reviews the only senior person’s work? Someone more junior, and it works better than people expect. They ask the questions nobody else will, and explaining your design to them tends to expose the weak part.