Code Reviews

There are different ways developers review someone else's code.
One option is to make sure that new defects are not being introduced. On the other hand, code reviews could be the most powerful and effective tool for knowledge sharing and learning.

There are important social and personal aspects related to code reviews. However, in this post I'll only concentrate on the technical side of the reviews: achieving high quality software.
In c# .NET space these are my 2 favourite books with heaps of tips about how the code should be written and structured:
- Framework Design Guidelines
- More effective c#

In addition, code should not pass a review if it violates any of the code smells. Originally it was described in Refactoring: Improving the Design of Existing Code.

Static analysis tools are very important in a modern software development. Tools like resharper shouldn't indicate any issues before code review starts.

Here is my list of items to validate during a code review:

  • Logging

    • Should be right amount and level to be able to analyse issues during qa or production.
    • Log on every exception handling .
    • Verbosity. For example, make sure there are no info level logs inside loops as too many information log messages would be difficult analyse.
  • Exception are used and handled correctly.

    • Exception should not be used to return a status or an error code.
    • Is custom exception required?
    • Analyze every line of code of what happens if exception is thrown here. How would the rest of the system behave?
    • Arguments of at least every public method are validated
    • InvalidOperationExceptions are thrown where coding assumptions are violated.
    • Custom Exceptions are marked with Serializable attribute and have default constructor.
    • Assess if over use of exception can cause performance issue
  • Multithreading/ concurrency

    • The minimal possible functionality is being locked
    • Locking on a right object (for example, no lock(this))
    • External code is not being called from inside lock
    • Threadpool is not abused
    • Long running functionality is executed on its own thread
    • UI elements are not potentially accessed from worker threads
    • No potential race conditions
    • Immutable types used
    • No performance issues as a result of locking
    • UI is not being blocked by functionality that runs longer than 50ms
    • If Thread.Sleep() is used, can it be replaced with something else, ei Timer, AutoResetEvent, etc
  • Networking

    • Service proxy is created immediately before the service call
    • Every integration point can potentially fail. Would the rest of the code be able to recover? What user experience will be like? Performance implications, etc?
    • Depending on the networking speed, each out of process call can potentially take much longer than in development environment. Same question here: What user experience will be like? Performance implications, etc?
  • Coding standards are applied.

    • All symbols names (ie methods, class, variables) make sense.
    • Avoid being too picky about minor styling issues
    • Business as well as technical assumptions or constraints are well documented. However, the obvious code shouldn't have any comments.
  • Design

    • Code is written in a testable way.
    • Single responsibility principle is applied to every class.
    • Inheritance is not used for code reuse, rather composition
    • Code is not duplicated or if not reused
    • Code is in a right project/folders
    • Architectural or design patterns are not compromised.
    • Dependency injection is used instead of Service Location pattern
    • No redundant code is left
    • Encapsulation is not compromised
    • Class or method should be marked as internal if they used only inside that assembly
    • Lazy evaluation - validate that some expressions are not evaluated more than once
    • out and ref parameters should be avoided
  • Unit test are available as well as an integration tests

    • Great to see code coverage before/after
    • Every integration point probably should have at least one integration test. However, sometimes it might be too difficult or time consuming to implement.
  • IDisposable pattern.

    • Using block is applied around disposable variable.
    • Try/finally block is used to set the final state of the object (analyse what happens if exception is thrown)
    • Whatever allocates disposable resource should be responsible of disposing it.
  • IEquatable interface is implemented where the objects might be checked for equality (ie keys in IDictionary)

    • If Equal is overriden, make sure operators == and != as well as GetHashCode() are implemented.

During the code review you don't have to fully understand the functionality. However, the code should be written in such a way that it wouldn't be a huge effort to any decent developer to quickly figure out even a complicated piece.

Please feel free to comment about other important items to validate. I will integrate it later in this post.

Posted by: Boris Lipschitz
Last revised: 03 Jun, 2012 09:22 AM

Comments

blog comments powered by Disqus