プルリクエストの書き方とレビューのコツ

A few weeks ago, I picked up Looks Good To Me by Adrienne Braganza (Manning, December 2024). It is 352 pages on code reviews. Most of it confirms things you probably already feel, but two chapters stayed with me long enough to write about: Chapter 2 on what a pull request should look like, and Chapter 6 on how to write review comments.What follows is the practical version of those two chapters. One engineer can adopt all of it on the next PR, without a team meeting first.The most useful framing I took from Chapter 2 is this: a pull request is a unit of communication, not a unit of code. The reviewer is your audience. Your job is to make their job cheap.A PR has two jobs:The title tells the reviewer what changed.The description tells the reviewer why it changed.Get those two right and you eliminate roughly half the back-and-forth in a typical review.A few rules:It must be self-explanatory without opening the PR.A linked ticket is not a title.Use a conventional-commit prefix (fix:, feat:, chore:) so the reviewer’s mindset is set before they click.Keep it under 80 characters.Take a real example. Watch the same title get better:csv export brokenCould be anything.fix DEV-2451A ticket reference is not a title.fix CSV export issue (DEV-2451)Generic, and the ticket belongs in the description anyway.fix: CSV export returns empty file because the date filter excludes records on the start day due to a strict inequalityOn the right track, but breaks the 80-char rule.fix: strict date filter drops records on CSV export start dayPrefix, specific, fits.If a teammate scrolls past your PR in a list, the title alone should tell them whether it concerns them. That is the bar.The description should answer the questions the reviewer is about to ask. That means:Context. Why does this change exist?Links. Ticket, design doc, related Slack thread.Abandoned alternatives. If you considered another approach and rejected it, say so. It saves a round trip.How to test, especially if the change is not trivially exercised by automated tests.A good description is one where the reviewer never has to find you on Slack to understand the change. Write it for the version of your colleague who has none of your context, because that version is real and is probably reviewing your PR.Code review is a three-party contract:The author. Be your own first reviewer. Re-read the diff before you assign anyone. Keep the PR atomic, reviewable in 10 to 20 minutes, ideally under 500 lines and 20 files. The number of times I have read a 1,500-line PR and rubber-stamped it because I gave up is greater than zero, and I am not the only one.The reviewer. You must actually understand the code, not just nod at it. Approving means you share ownership. If something does not make sense and the explanation is “I trust them,” do not approve. Ask.The team. Document explicitly when the process can be skipped. Hotfixes are real. Pretending they aren’t real produces shadow processes, which are worse than documented exceptions.Braganza puts it neatly:Code reviews are like turning signals: optional, but they reduce crashes.So far, so good. Now the second half.A PR with a great title and description can still produce a miserable review if the comments are dismissive, ambiguous, or vague. Chapter 6 is the toolkit.Be objective. Talk about the code, not the person. Think in “we”, not “I” or “you”. Before you post the comment, ask yourself why you want to make this suggestion. If the answer is “because I would have written it differently,” reconsider.Watch your language. Ask, do not command. “Can we” instead of “you should.” This is not about being precious. It is about not putting the author on the defensive in a forum where everyone on the team can read along.Add context. Every non-trivial comment should explain why the change matters. “Because that is how we do it” is not a reason. “Because we agreed to migrate everything off v2 by Q3, link” is.Be specific. The author should never have to guess whether your comment is blocking or a stylistic preference. That is what comment signals are for.A small vocabulary that costs nothing to adopt:needs change. Blocking, small change.needs rework. Blocking, large change. Maybe discuss offline.nitpick. Non-blocking, style or preference.levelup. Non-blocking suggestion for improvement.A codified version of this idea lives at conventionalcomments.org. Whether you adopt that exact spec or not, having any agreed vocabulary in front of your comments removes a class of misunderstanding entirely.Structure every non-trivial review comment as:Request. One sentence on the what.Rationale. One or two sentences on the why.Result. A measurable end state the author can compare against.Take a comment I might leave on a batch job:needs change: Can we wrap this database call in our withRetry helper? The direct call will fail the whole job if Postgres has a transient hiccup, which we have already seen twice this quarter (link). Our other batch jobs use withRetry for the same reason; adding it here keeps retry behaviour consistent across the codebase.The signal is up front, so the author knows it is blocking. The request is phrased as a question. The rationale gives a concrete, link-backed reason. The result is verifiable: the call uses withRetry and matches the rest of the batch jobs.The same suggestion, evolving from worst to best:You should not hard-code 86400 here.Direct command. “You” puts the author on the defensive. No reason given.We should not hard-code 86400 here.Shared ownership. Tiny change, real difference.Can we avoid hard-coding 86400 here?A question invites discussion instead of demanding compliance.needs change: Can we avoid hard-coding 86400 here?Now the author knows it is blocking, not a nit.needs change: Can we replace 86400 with a SECONDS_PER_DAY constant in time.kt? The same value is already hard-coded in two other files (links), and a named constant makes the intent obvious without needing a comment.Specific suggestion and reason included. The author knows what to change, why, and how to verify.Each step costs the reviewer maybe ten extra seconds. It saves the author a back-and-forth. Run that math across a year of reviews and the difference is enormous.If you only take a few things away, take these:Title is the what, description is the why. 80 characters, prefix, no naked ticket numbers.Be your own first reviewer before you assign anyone.Approving means you understood it. If you didn’t, ask.Comments: think “we”, ask instead of command, add context, signal whether you are blocking.For anything non-trivial, write Request, Rationale, Result.None of this requires team alignment. None of it requires a process rollout. It is one engineer changing how they write a title, a description, and a comment. The compounding effect is what surprised me.That said, Looks Good To Me has more in it. A chapter on team working agreements, an emergency playbook for when production is on fire and the review process is in the way, and an honest chapter on AI in reviews. If your team gets value out of these two chapters, the rest is worth the read. But you do not have to start there. Start with the next PR you open.If your team has worked something like this out, I would love to hear how it landed in practice. Thanks for reading. 😊

※ 著作権に配慮し、引用は冒頭3段落までです。続きは元記事をご覧ください。