Review Your Own Code Review
I’ve talked about code reviews before. There’s are things you should do when you review code, and there are things you shouldn’t do. Most of those things are about what to do when reviewing someone else’s code. The rest are about what goes into preparing a good code review.
I stand by those ideas. But there’s on important thing that goes into preparing a good code review that I haven’t mentioned in the past. That’s reviewing your own code reviews, before asking anyone else to look at them.
One of the biggest reasons is to know for sure exactly what is going into the review. In order to write a good title and summary, you need to know what you’re summarizing. You want to be clear about what’s included, and for that you need to know.
It gives you a chance to see how many “ands” you put into your summary. Very often you find that to do the thing you set out to do you need to make a bunch of other changes to make it possible. Since each review should do one thing, the self-review is a good time to catch that you should split the review up into multiple reviews. NB: Your reviewers will thank you.
Since you’re looking at the entire thing at once, are you being consistent? Are you being readable? Do things visually flow? Not just formatting, you have rules and formatters for that (don’t you?), but in structure and naming. Do you call something inventory in one place and stock in another? Make them the same. Did your method grow to 300+ lines? Break it up. Are you calling methods from hither and yon? Do you need to jump around/across files to follow what’s happening? Consider bringing things physically closer together to make reading/debugging easier.
Did you leave yourself a note to looks at something related later? I do that all the time since I don’t want to change contexts and track something down. I also often forget that I wanted to track that thing down, so seeing the note during the review gives me another oportunity to follow through.
Another thing you get from self-review is the chance to notice any debug helpers you left in the code. Things like extra print or log statements, parameter overrides, or changed defaults. Once you find them you’ve got an opportunity to change things. Should those prints be log messages? Should the logs be at a different level? Are you changing or adding a default that needs to be documented?
Speaking of documentation, this is also a good time to check and make sure that there is appropriate documentation for what you did, why you did it, and why you didn’t do something else. You’ve already done it once, but this is your chance to look at the changes in totality and make sure.
What about unit tests and/or code coverage. Do you have enough? Neither are guarantees, and you hopefully have automated systems that will check this for you, but being a good neighbor means the code under review is done. If you have folks do a review its better if you’re sure all the existing tests still pass, and any needed tests have been added. If not, you’re going to have tyo go back to your reviewer and ask them to review things all over again once you fix those problems. That is not the way to make friends.
What it comes down to, and what all these benefits add up to, is the chance to make sure your change is done and really ready for review before you send it on to others. Between context switches and the time taken to do the review, you’re about to cause multiple man hours of work across multiple people. It’s on you to do your homework first.
