Self Code Review Methodology

When I worked as a team leader I didn't want any code to get into the product without the code review in addition to the unit tests added. But good code review takes time, so I started thinking how I can optimize the process...

First, I came to a conclusion that a review actually consists of two parts:

• Code logic. This is best done by challenging. If the developer quickly and correctly answers to questions, he probably thought about that scenario and covered it. If not, we open the relevant code and check its logic in depth. This gives the opportunity to concentrate on less 'polished' code, analyze it deeper and correct more issues.
• Code sanity, which includes:
  • Code format - ensured by IDE.
  • Code style - which actually is a routine check list. Bingo! Why not have this check list written and given to the developers to do themselves to save the reviewer's time?

Below is the check list I created, please leave your comments...

For each added field

1. Consider type safety. I.e. when the field is used, no cast is required.
2. Ensure its access modifier is private.
3. Ensure its value cannot be computed by other means (using other fields/methods). If yes, it means this field is for caching purposes or used by some setter. If it's for caching:
   • Ensure this caching is required and comment this in code.
   • Ensure it is always in sync with computed value and comment how this is achieved in code.
4. Consider marking this field 'readonly'. If not possible, consider refactoring resulting this field becomes 'readonly'.

For each added property

1. Ensure it has a getter.
2. Consider removing a setter.
3. Ensure set/get parity, i.e. if some value is set, the same one is get.
4. Ensure sequential gets return a same logical value.

For each added property/method

1. Consider minimizing its access scope (private static <--> public virtual)
2. Ensure all the arguments are checked and relevant exception are thrown.

For each added class

1. Consider minimizing its access scope (private sealed inner class <--> public not sealed).
2. Ensure it has a minimal set of constructors.
3. Consider refactoring to have one constructor with initialization logic and other forwarding to it. If needed, add a private constructor.
4. If there is an override for Equals/Hashcode methods, ensure they come in pair and both are computed from the same data.

For each local variable

1. Consider type safety. I.e. when the variable is used, no cast is required.
2. Minimize variable scope.
3. Ensure proper clean up in finally blocks; usually initailization should be right before the try block and not inside it.

For each added line of code

1. Consider implicit impacts (boxing, objects creation, computation).
2. Ensure that every System.SystemException derived exception can be possibly thrown is by intention. (NullReference, Cast etc).

For each 'lock' statement

1. Consider using a framework class, which has the required synchronization built-in. For example, if you need a synchronized Hashtable, don't do locking yourself, but create it with Hashtable.Synchronized() / SynchronizedKeyedCollection<K, T>.
2. Otherwise ensure the design is discussed with your manager.

ASP.Net Unit Testing

Some tell there are many solid solutions by HP (Mercury) or Microsoft's TeamSystem, some claim that all those are not Unit, but Integrative tests. And there are many that mostly perform manual testing... Who is right and what is the best approach?

As usual, let's start from the requirements from the testing framework.

1. Granularity. We are talking about unit testing, the name suggests that we want to test application units, separately.
2. Ability to automate. Definitely we want an ability to run the tests automatically, for example after a build. This requirement also yields a requirement for results stability through the application life cycle.
3. Coverage. Every part that is not covered by a unit test joins the risk group of deferred bugs. Even if there is integrative testing, it probably does not run frequently and therefore our control over correct system status reduces when the un-unit-tested group grows.

Let's partition our application and see how we can test every part.

1. Libraries, that do not require any "web context". Here we can simply write plain unit tests. No problems.
2. Custom and User controls. In fact, those are also libraries, providing a well defined API, that can be systematically tested... But how to do it if they require "web context"?
3. Pages, pages, pages, which actually produce our application on the server side and...
4. Client side logic - let's not forget all the Java Script we run on the client browsers.

Unfortunately most of the "well known" solutions, including those mentioned at the beginning address mainly point 3 above. The common pattern is to record the requests/responses sent/returned by the server; later sent those requests back to the servers, receive the response and validate it again previously recorded one.

This introduces several problems:

1. Granularity (since 'Custom and User controls' part is handled on the 'Page' level). When the test fails, we know that the Page failed, not some specific control and therefore had to invest more time in regression diagnostics.
2. Ability to automate. Since the page evolves over time, our old recording may become invalid, even if everything is correct. This produces many false alarms, preventing automation.
3. Coverage. Client side logic is left untested.

In other words we have a problem in every aspect of our requirements, what can we do?

1. Re-introducing test-driven development. Since not every code can be easily tested, we must include its testability into the very beginning of its design phase. The rule of dumb says that testability reduces when we move forward over the following path: 'Simple Library' -> 'Custom/User control' -> 'Page' -> 'Client Side logic'. This means that we must strive to move our application code as 'left' as possible.
2. Create unit tests for our Custom/User controls. We use to create a separate test for every class in 'Simple Library', the same we can do with controls. Once we have a dedicated test page per control, it does not change when the application changes, improving our 'Ability to Automate' over time.
3. Client Side logic testing. Since client logic runs in the browser, in order to test it, it must run in the browser, period. Any other option simply does not do the job. One of the best tools I know to run tests on the browser is Selenium - it will be covered in the next article.

Summary

1. Partition your application as described above.
2. Try to move as much code as possible 'up' within the partition.
3. For each User/Custom controls write a dedicated test page to test its functionality.
4. Use some tool for testing client side logic, my recommendation is Selenium.
5. If you go with Selenium for client side logic, for consistency use it also for Pages testing.