by Serguey Shinder
It came in from a contractor we had used before and liked, about four hundred lines, a new endpoint and the service work behind it. I reviewed it on a Tuesday morning with a coffee and approved it in roughly twenty minutes, which for four hundred lines is fast, and I remember feeling efficient rather than careless.
What made it fast was that it looked exactly right. Our naming conventions. Our error type. The small internal result wrapper we use instead of exceptions across that boundary, used correctly, including the awkward case that everyone new gets wrong for their first month. The tests were arranged the way our tests are arranged. Even the comment style matched, and we have an unusual comment style.
Two weeks later it behaved badly in production under a condition none of the tests covered, and when I went back and read it properly, line by line, the logic in one method did not make sense. It was not subtly wrong. It was structurally wrong, in a way I would have caught in five minutes on any ordinary Monday.
I had not read it. I had recognised it.
That is the part I have been turning over since. My review process, the one I would have described with some confidence if you had asked me about it, was not primarily an examination of logic. It was a search for signals that the author understood our world. Right names, right idioms, right shape. Those signals used to be expensive. You could not produce them without having read a great deal of our code, and having read a great deal of our code correlated strongly with having thought about the problem. Style was standing in for thinking, and for twenty years that was a reasonable trade.
It is not a reasonable trade now, because those signals have become nearly free. Something that has seen the repository can reproduce house style perfectly while holding no view whatsoever about whether the code is correct. Fluency and understanding have come apart, and my instincts were built entirely on the assumption that they travel together.
I have not solved this. What I do is cruder and slower. I read the hardest method first, before I have formed any impression of the author at all, and I try to state in my own words what it is supposed to do before I look at what it does. If I cannot, that is the review comment, however polished everything around it looks.
I also stopped treating a clean-looking diff as evidence of anything. It used to be weak evidence of care. It is now evidence of nothing, and the twenty minutes I saved that Tuesday cost us about three days.
– Serguey Asael Shinder
Leave a Reply