What you'll learn
Quick Answer
Review your own diff first and leave comments explaining anything surprising, because the fastest way to a good review is a reviewer who is not confused. Keep pull requests small and single-purpose. As a reviewer, look for correctness at boundaries, missing authorisation checks and silently swallowed errors, and let automated tools handle formatting. Label comments as nit, question or blocking so the author knows what actually stops the merge.
Review your own PR before anyone else does
Open your own pull request, read the diff line by line, and fix what you find before requesting a review. This one habit changes how people perceive your work more than any other, because reviewers stop finding the obvious things and start finding the interesting ones.
The diff view shows you things the editor hides. A console.log you added while debugging. A commented-out block you meant to delete. A file you renamed but only half updated. A stray .env or a hardcoded API key. An unrelated formatting change across two hundred lines because your editor reformatted a file on save, which alone can make a review impossible.
git diff --stat main...HEAD # what did I touch, and how much
git diff main...HEAD # read the whole thing
git add -p # stage in pieces, review as you go
Use three dots, not two. git diff main..HEAD compares the current tips, so it includes everything that landed on main after you branched, and a busy repository makes your ten-line change look like six hundred. git diff main...HEAD compares against the merge base, showing only your changes. That is what the reviewer sees on GitHub, so it is what you should read.
Then leave comments on your own diff. If you took an odd approach, say why on the exact line. If a chunk is generated or copied wholesale, say so, so nobody reads it closely. If you know a part is weak, ask about it directly: "I am not sure this retry handles a timeout correctly, could you look at this bit specifically?" Reviewers give better attention when they are told where to spend it, and admitting the weak spot is read as competence rather than doubt.
What makes a PR easy to review
There is a well-known pattern in engineering teams, sometimes called bikeshedding: people comment in proportion to how easily they understand something, not how important it is. A pull request that changes the database schema, adds a background worker and renames forty files gets "LGTM" because nobody can hold it in their head. A ten-line function gets a long thread about naming. The fix is not to nag reviewers. It is to send work that can actually be reviewed.
One purpose per pull request. If your description needs the word "and" twice, it should probably be two PRs. Fixing a bug is one. Renaming a module is one. Doing both means the reviewer cannot separate the risky change from the noisy one, and if the release has to be rolled back, they come out together.
Separate the refactor from the behaviour change. Move the code in one PR that changes nothing, then change the behaviour in a second where the diff is five lines. Reviewers can verify a pure move quickly and then concentrate on the part that can actually break.
Write a description that answers three questions. What problem does this solve, how did you solve it, and how did you check it works. Link the ticket. If it touches the UI, paste a screenshot. If it touches an API, paste the request and response. Two minutes of writing saves twenty minutes of back and forth across a time zone.
Fix duplicate certificate on double submit
Problem: clicking Download twice within a second minted two certificates
for the same enrolment, because the uniqueness check and the insert were
separate queries.
Fix: single INSERT with a unique index on (user_id, course_id) and the
conflict handled as "already issued".
Tested: added a test that fires two concurrent requests; also verified
manually on staging with the network throttled.
Finally, get your own CI green before asking for review. A failing pipeline means the reviewer is reading code you are about to change anyway, and nothing burns goodwill faster than a review that has to be repeated.
What to actually look for
Read for intent before you read for detail. Does this change do what the description claims? Does it need to exist at all, or does something in the codebase already do it? Those questions are worth more than every style comment combined, and they are the ones only a human can answer.
Then work down a mental list, roughly in order of what would hurt most in production:
- Correctness at the boundaries. Empty list, null, zero, negative, a duplicate submit, a very long string. Most bugs that reach users live at an edge the author did not picture.
- Authorisation, not just authentication. Being logged in is not permission to see this particular record.
- Errors that vanish. A bare
except: passor an emptycatchblock turns a loud failure into silent wrong data. - Data and migrations. Can this migration be reversed? Does it lock a large table? Does it delete a column something still reads?
- Queries in loops. The classic N+1: fetch a list, then hit the database once per item. Fine with ten rows in development, fatal with fifty thousand.
- Tests that assert something. A new test that calls the function and checks nothing is worse than no test, because it looks like cover.
Here is the kind of thing that is worth every second of a careful review:
@app.route("/orders/<order_id>")
@login_required
def get_order(order_id):
order = Order.query.get(order_id)
return jsonify(order.to_dict())
The decorator proves the caller is logged in. Nothing checks the order belongs to them. Change the id in the URL and you are reading a stranger's order. This is an insecure direct object reference, it passes every happy-path test, and the fix is one line: Order.query.filter_by(id=order_id, user_id=current_user.id).first_or_404().
Similarly, string-built SQL deserves a blocking comment every single time:
cur.execute(f"SELECT * FROM users WHERE email = '{email}'") # injectable
cur.execute("SELECT * FROM users WHERE email = %s", (email,)) # parameterised
What you should not spend review time on is spacing, quote style and import order. Put a formatter such as Prettier, Black or gofmt and a linter in the pipeline, and the whole category disappears from human conversation.
Feedback that helps instead of stings
Review comments are read without tone of voice, often by someone tired, sometimes by someone new to the team. "Why did you do this?" is a genuine question in your head and an accusation on the screen. The single most effective change is to comment on the code and never on the person: not "you always forget null checks" but "if user is null this throws at line 42".
Label every comment with its weight so the author knows what actually blocks the merge:
nit: `getUserData` returns a list, so `getUsers` might read better.
Not blocking.
question: what happens here if the payment webhook arrives twice?
I might be missing something.
blocking: this returns any order by id, so changing the id in the URL
exposes another user's order. Needs a user_id filter.
praise: nice catch extracting this into a helper, the callers are much
clearer now.
Without those labels, ten equally weighted comments read as ten problems, and a junior developer concludes the whole PR was bad when nine were preferences. With them, the author reads one blocker and moves quickly.
Ask rather than instruct when you are not certain, and say why when you are. "Use a set here" is an order. "A set would make this lookup constant time instead of scanning the list each iteration, and this runs per row" teaches the reason, and the author will apply it themselves next time. The reason is the whole value of the review; the fix is incidental.
Say what is good, and be specific about it. "Good test names" or "thanks for splitting this out, it was hard to read before" costs nothing and makes the blocking comment beside it land as help rather than criticism. Reviewers who only ever comment on faults train people to dread review, and people who dread review send bigger, later, riskier pull requests.
Two more rules that prevent most conflict. If a thread reaches three replies, stop typing and get on a call or walk over; text is a poor medium for disagreement about design. And review promptly, because a pull request sitting for three days blocks a person, invites merge conflicts, and quietly signals that their work does not matter.
Receiving review and disagreeing well
Being reviewed is harder than reviewing, especially in your first job when you are already unsure whether you belong. The reframe worth making early: comments on your pull request are the cheapest feedback you will ever get. A reviewer is spending their afternoon making your code better before customers see it, and getting twenty comments on your first PR is completely normal.
Reply to every comment, even briefly. "Fixed in 3a1c9f2" or "good catch, done" closes the loop and lets the reviewer see progress without rereading the whole diff. Silence forces them to check each thread by hand, which slows the next round.
When you disagree, disagree with evidence rather than with feeling. "I chose a list here because the collection is at most five items and it keeps insertion order, which the report depends on. Happy to switch if you still prefer a set." That is a technical position and either the reviewer has more context or you do; the conversation ends quickly with someone learning something. Compare it with "it works fine", which ends nowhere.
Sometimes the reviewer is simply wrong, and that is fine. Ask for the specific case: "what input would break this? I want to add a test for it." Either they produce one, and you have found a real bug plus a test, or they realise the concern does not apply. Either outcome is a good use of five minutes.
If a comment feels harsh, assume tempo rather than malice. People type curtly between meetings. Answering short comments warmly usually resets the tone immediately, and if it does not, that is a pattern for a private conversation, not a public thread.
One last note for students. You can do all of this before your first job. Open a pull request against your own project, write the description properly, review your own diff, and leave real comments. Then review someone's PR in an open source repository, starting with the easy honest kind: a broken link in the docs, a missing null check, a confusing name. It builds the reflex, and "reviewed pull requests" on your profile is far more convincing to an interviewer than another tutorial clone.