Code Review 101

My first code review was at my previous co-op placement. Let's call the place D12.

The only flaw with the code review was that the project that I was working on was useless. The task was to build a web service that sat between the bug tracking database and the web app front end. Hazzah. Two weeks I had built a web service that did the basic functions. One week later I had some crappy authentication to go with it. I spent a long time attempting to make it such that it'd take from the windows authentication but I can't remember exactly why it just straight up wouldn't work. I wasn't ecstatic with the code. It was pretty half-assed when I finished but when it's busy-work, you're just happy you reached a set deadline.

Then I sat down with the head of development (I think he's a program manager). First we went through a demonstration of the main functions. Using my four apps (.net web, .net forms, asp, vb 6.0) for adding a bug, editing a bug, displaying bugs, filtering the bugs, and all the other basic things you can expect. Then he finally got me to open up the web service API.

"Why is that parameter a string and not an integer?"
"Why should I have to jump two levels?"
"Why do [...] isn't that too large a hindrance to performance?"
"What are the advantages of taking out parameters and using an object instead?"
"How would you propose we print out statistics for bugs using your API?"
"What if we wanted to extend it out a bit more?"
"Why the hell is there camel case, hungarian, and regular named variables?"

Most of them were completely legit issues. I couldn't defend myself for them. My only real reason was, "I thought you just wanted me to keep busy?" Yeesh.

"Why is that still a string?"
"Is that the best you can come up with to get stats?"
"Should we bother doing a data warehouse?"
"Your API naming scheme is not obvious enough."

The second time was probably as rough but less relevant as the first. The first one made me put some perspective into the point of what I was doing and I actually weighed certain aspects of the project. The second portion felt like a rip. Many of the questions were a bit absurd and he kept railing on my justifications. To challenge a coder to think is one thing but he went too far. The most painful part of the second code review was the fact that he said my code would not be fit to be put into production. Firstly, I've seen the code in production and to compare that to mine is insulting. Secondly, that statement I felt was unjustified for the second round. It'd be useful to say, "your code isn't ready for code because you need to work on X." Instead I had a really empty statement. I think I responded with, "You're kidding me, I've seen the code that goes in." and also, "I spent 3 months looking for standards and things to follow and got nothing ... what the hell." I think my only qualm was that it felt more personal to me than constructive. Please note that I did not say my code was adequate.

My final code review was not with the head of dev but an architect at D12. Thankfully this one was a lot more pleasant. His questions were a lot more civilized. I had notes and justifications for almost everything: the parameters, the naming conventions, all functions, every unclear section of code, and I even had categorized the calls for different users. This doesn't mean it was any less intensive because the code review lasted for about 2+ hours. He railed on me the same fashion as the head. Except this time he was reasonable about my justifications.

Most people would be under the impression that I'd hate code reviews after that experience but nothing could be further from the truth. It was awesome. I learnt what considerations of I should look for during development. I also learnt that I should fight tooth and nail for code I think is good.