In my new role at Brand Integrity as lead developer, I am tasked with the responsibility to perform code reviews on our platform. It’s one of my favorite parts of the job (besides the fame, fortune and socialite lifestyle) because not only does it help the platform, it helps me become a better developer as well.
If you look up “how to perform code reviews”, you can find a plethora of blog posts on how to perform and survive code reviews. I’ve decided to highlight a few things to share that I have learned from my experiences which I believe are very important.
Consider doing the review in two pieces: one together and one separate. Generally whenever I code review, it’s mainly because an important module has been changed or a developer is working through a unique solution. It takes time to understand approaches. I’d suggest as a reviewer, take a look at the code before meeting together and write down anything you would like to review. Once you get together, you can have a healthy dialog about how a particular piece of code was approached, rather than wait 15 minutes while the reviewer goes traverses the source code.
Only make minor changes, don’t make major changes. It is the developer’s responsibility to fix and rework the code based on the review. If there are simple code guideline changes, it’s okay to make those changes as long as the developer is aware of it (it’s just easier and quicker). Make sure before you end your review both the developer and reviewer know what is expected to be completed.
Don’t do surprise reviews. I just think this is a terrible practice. I did it once and felt bad after it. Allow someone to decline the invitation. Sometimes someone just isn’t ready for a review on a particular module.
Make a list of priorities specific for the review. Everyone involved in the process (the reviewer and developer) should be aware of what is important within the review. While developers should not code just for what is in the review, it’s good to know what you as a reviewer are looking for. This can be different between reviews, and it should be reflective on what is important to the development department as a whole.
For Those Being Reviewed
Have pride and confidence in your art. In most blog posts, you will see “don’t take it personally” a lot, and generally as one of the most important points. I respectfully disagree with the advice. It is true that you shouldn’t be emotional in a code review and think any recommended changes as a personal slight, but have pride in your work and you SHOULD take it personally! Have faith in yourself as a developer and explain why you did it your way. As a reviewer, I may have not have realized what you’ve done (after all, I’ve only looked at your code for a small time) and you may be right.
If you are the one reviewing work, recognize you may be treading into sensitive territory, and recognize that everything is generally written for a reason.
Be prepared to justify your work. Be ready to defend what you did. You wrote it and it is yours. If you are in a situation where you are truly not sure if what you did is the right approach, still try and justify your solution because it may still be the right path.
Coding in General
Make functions do precisely one task. One thing I’ve learned as a reviewer (anyone’s code, including mine) is that it is infinitely easier to review code which has been structured where functions have a single and specific purpose. Of course, this seems like “well duh” but if you do a lot of UI coding, it’s easy to forget. One of the worst offenders is within code behind development on .NET pages. It’s easy to put all functionality within the event handler. It’s obvious what btnSubmit_Click does within the context of the control.
Given little to no context however, it’s difficult to review and make design decisions. Having the general knowledge of the system, I don’t remember what btnSubmit_Click on page X vs. page Y does. I may remember however, that page X and page Y has a function called SaveUser. Because of the duplicate functionality, it’s obvious that both page X and page Y should share the same SaveUser function and its functionality should be pulled outside of the view tier.
Comment frequently but don’t write a novel. I feel that young developers, especially those who went to school, are beaten over the head about creating a lot of comments. I think this is much better than no comments. However, it misses the point. The BEST form of commenting is to program your code to be self commenting. This means that variable names must be clear and concise, the flow must make sense and be as simple as possible and the code formatting must be consistent and standard. This is FAR better than a ton of comments. As a reviewer, it’s more important for me to read the code than the comments so I can make better suggestions.
If it feels wrong, it probably is. Go get it reviewed. Sometimes you write code which just doesn’t feel right. You are probably right to feel that way and something just isn’t right. Get your code reviewed before it goes into QA or even into production. However, come to the review with a description as to why you chose your approach.