|
I had an experience in my first year of grad school that has informed my code reviews and my general attitude toward reviewing others' work ever since. I was taking a graduate-level introduction to mathematical logic. In the class we learned about a bunch of different propositional axiomatic proof systems, and then we proved properties like soundness and completeness for them. The class was organized around class participation: everyone was required to do at least one proof at the board in front of the class and every session consisted almost entirely of students doing proofs at the board. I'd been out of school, working, for eight years before grad school and I found this all very intense and unfamiliar. I did my required work at the board, choosing an easy problem, but otherwise mostly sat in the back and watched. There was, however, a cohort of very intense aspiring logicians who sat in the front row and took deep interest in every proof. One day, a student went up with a particularly challenging problem. From the start it was clear that he was taking an unorthodox approach to the proof. The front row went nuts, interjecting and kibitzing on practically every line, telling the guy different things that he should do instead of whatever it was that he was doing. The guy was a good sport and tried to justify everything and answer all the criticism, but it seemed like he might not get through the proof before the class ended. The prof who had been watching quietly from the side of the class, let it go on for a while, then stepped out and quieted everyone. "I need you to remember," he said to the front row, "that we are the audience. It is not our job to prove this theorem; it is this gentleman's job. Our job as the audience is to listen to him, and at the end, decide if we are convinced. He may do things differently that you would have done, but that doesn't matter, as long as in the end we are convinced." That was twenty-three years ago, but it stuck with me. In programming, there are often many different ways that a problem could be solved, but there is almost never One True Way™, even though there are often many voices that would like to convince you that their way is the right one. My job as a reviewer of code, or really anything, is not to try to get the other person to do what I would have done, but to decide if I'm convinced that the way that they've chosen is good enough, and, if not, to ask for changes that would convince me. Sure, sometimes a PR is so bad that it needs to be completely rewritten, but in my experience that's rare, and in that case the feedback should probably be given in person and in private or in a friendly, sympathetic group. |