Saturday, February 26, 2005

Unit testing private methods using reflection

Don't know how many other people have come across this problem, but a couple of days ago I was writing a class in C# and I decided I wanted a private method to perform some functionality. I knew the private method would contain some non-trivial logic, but didn't want to expose it to other classes that other developers were working on. I decided that the logic within the method was serious enough to warrant some unit tests, but how could I unit test considering the method was private. Normally I would make the argument that as long as I tested the public interface correctly, then I should be fine, however, I had a suspicion that this time was different, so I sought the advice of my fellow Readify colleagues, and the ever brilliant Mitch Denny came to the rescue with some sample code that used .net reflection to call into the private method.

TestVictim victim = new TestVictim();
Type victimType = typeof(TestVictim);
MethodInfo method =
BindingFlags.NonPublic BindingFlags.Instance);

//pass parameters in an object array
method.Invoke(victim, . . . .);

or in my case more along the lines of

Type victimType = typeof(VictimClass);
MethodInfo =
BindingFlags.NonPublic BindingFlags.Static);

//pass parameters in an object array
method.Invoke(null, . . . .);

as this was a static private method.

This worked a treat and I very quickly discovered the reason that I'd suspected I should go the extra mile and create this unit test. The very first time I ran my unit tests with just the most basic tests in place, I discovered one of those "doh" type bugs. Had I been attempting to just test the public interface, because there was considerable complexity in the public method that was using this private method I would have been looking in the wrong place for ages trying to debug the public interface, stepping into each line etc..., but because I'd spent 15 minutes setting up the unit test, when I did finally write the unit test for the public method, it worked first time.

Question: So when should you go to the extra effort of using reflection to unit test a private method?
Answer: When the private method contains non-trivial logic, and writing tests for the public interface won't make it obvious what's really happening or you won't be able to get decent code coverage from the public interface tests. There may be other times as well, but this is the rule I'm going to stick to for now.

Wednesday, February 09, 2005

Peer Code Reviews - 2

In my previous article on Peer code reviews I mentioned I didn't like the idea of insisting on a code review before checking into source safe. Well, I still don't like it, but I have discovered one reason which supports the idea.

If developers are left to code and check in/check out as much as they like, and are only required to have a code review before a periodic label, then it becomes tempting to write large chunks of code before getting a code review. now if a reviewer, by definition another developer, is asked to review pages and pages of code, then as the reviewer is also undoubtedly a busy person, they will simlpy use the page down key to scan over vast amounts of code and not really look for anything in particular as to look intently at each peice of code requires significant effort. I guess the conclusion is review early, review often. However, I still believe that there should be very few barriers to checking in building code.

Wednesday, February 02, 2005

Peer Code Reviews

Even though I have had 8 years experience in the IT industry, it may or may not surprise you to know that until my current job I have never had a formal peer code review. In the past the only times that a peer has looked at my code has been in a very informal way, and not really looking for the kind of things that should be looked for in a proper peer code review. So as you might guess, I was a little nervous when faced with the prospect of a peer code review, however, I am now completely sold on the idea.

I believe there are 2 elements that make code reviews invaluable to good code production. Firstly, the reviewer has a different perspective on the code. The reviewer is someone with different experiences, and as they are not in the thick of the coding, have time to relax and view the code in a more objective way than the creator of the code. They are not subject to the same line of logic that the coder has gone through to produce the code, so if the coder has written something a certain way after a string of incorrect or sub-optimal logic, the reviewer is free to question the logic, or propose a better way of implementing an algorithm, and the coder then is forced to either justify his logic by tracing through his steps, validating his decisions at each point, or accept the better way and re-code accordingly. The reviewer can also check for consistency with the accepted coding standards, and if the coding standards are good, better adherence will result in more readable, more maintainable code.

The second element is the psychological factor. As a developer, if I know my code is going to be reviewed, I will be very careful to adhere to the accepted coding standards and I will make sure that my code is set out in such a way that it is easy for another person to read. I will also validate my logic, imagining how I will explain complex bits of code to my potential reviewer. And finally I will do a once over of my code before I ask someone to review it, tidying everything up, making sure the review process will go smoothly and there will be no serious issues that the reviewer could potentially pull me up on.

All this doesn’t guarantee bug free code, in fact, the purpose of a code review is not to replace a proper QA process, but it creates an opportunity to catch obvious coding errors that would have otherwise come up in the QA process, and would have extended the whole phase unnecessarily. As the QA cycle is always the largest unknown of any software project, anything that can potentially decrease this phase has to be a good thing.

The only thing I don’t quite agree with is the way in which they use their source control in conjunction with the peer code reviews. Their idea is that there should not be a single line of code under source control that has not been reviewed. This creates an obstacle to checking in regularly. My personal opinion is that there should be very few obstacles to checking in modified source code. At my last work the rules I instated were just the obvious two; only check-in if the project is building (ie don’t break the build), and check-in regularly (usually before you go home at night). I always get nervous if source files are checked out for too long, there are obvious problems that can occur. The issue is that to avoid large amounts of small code reviews developers will wait for multiple days until they have some serious code to review. I think that all though the concept is noble, maybe a similar concept could be achieved by having some simple processes around labeling (or tagging depending on the notation of your source control), that can be administered by the release manager. I'll have to see how the process goes over the next few months, and see if my opinion changes with respects to their source control methodology. One things for sure I'm a convert to peer code reviews.