My first real pull request was 400 lines of a billing helper, and I was proud of it. It worked. The tests passed. I had even added a docstring. I opened the PR on a Friday afternoon and went to get coffee, already imagining the approval.
I came back to 23 comments.
Nobody was rude. That was the worst part. Every comment was calm, specific, and correct, which meant I had no excuse to be annoyed. I just sat there reading my own code through someone else's eyes, and it looked like a stranger wrote it. Four of those comments still change how I write code today.
"This function does three things"
The function was called process_payment. It validated the card, calculated the refund, and sent the confirmation email. I thought that was efficient. One function, one call, everything handled.
The reviewer wrote: "This function does three things. When the email fails, the refund has already happened, and the retry will charge the card twice."
I had never thought about what happens halfway through. In my head the function either worked or it didn't. In reality it could die between step two and step three, and then the customer gets charged, gets no email, and the retry charges them again. The fix was boring: three small functions, each able to fail on its own. I have split functions apart ever since, because I can still picture that double charge.
"What is data2?"
I had a variable called data2. There was also a data. The reviewer just wrote: "What is data2?"
I knew what it was. It was the refund amount after the rounding fix, as opposed to data, which was the refund amount before the rounding fix. All of that lived in my head and none of it lived in the code. I renamed it rounded_refund and the whole block suddenly made sense without a comment. That was the day I learned naming is not decoration. A bad name is a bug that hasn't happened yet, because the next person will guess wrong about what data2 holds, and they will guess with confidence.
"This test only tests the happy path"
My test created a valid payment, processed it, and checked the total. Green tick. The reviewer wrote: "This test only tests the happy path. What happens when the card is declined?"
I had no answer. I had never run the decline case. I wrote the test, watched it pass, and moved on. The reviewer was pointing at something I still catch myself doing: testing the code I wrote instead of testing the code that could break. Now I write the ugly test first. Declined card, empty list, zero amount. The happy path is the least interesting thing my code does.
"Did you run this?"
This one stung the most. The reviewer had found a typo in a branch I never executed: a variable I renamed in one place and not the other. The code would have crashed the first time a refund exceeded the original charge. My tests never hit that branch, and I had never run that branch by hand either.
"Did you run this?" Four words. I had run the tests. I had not run the code. There is a difference, and I had been pretending there wasn't. Now, before I open a PR, I run the actual flow myself, with my own hands, including the branch I think nobody will hit. Especially that branch.
What I got wrong about reviews
I went into that PR thinking a review was a grading session. What actually happened was slower and better: 23 small corrections that added up to a different way of writing code. Nobody taught me a principle. They just kept pointing at specific lines and asking specific questions until the principles showed up on their own.
The PR took two more days to merge. I rewrote half of it. The second version was shorter, duller, and better in every way. It had small functions with honest names, tests for the cases that could actually break, and no branch I hadn't run myself.
Top comments (0)