Geeks With Blogs
Marcin Celej blog

I've been doing a lot of code review lately. I've been doing it with every developer personally. And then I realized I can do an every week, one hour long public review. Without any names: just me, code, and bunch of developers sitting around the projector. It occured to be a fantastic way to spread knowledge in a team. I'd like to share main points I spotted during the public code review.

Two levels of code review:

  • Syntactic level
  • Logical level

I usually concentrate on the first level but I always try to figure out what is the logic of a method that I investigate. If I cannot understand from the code what the method does it tells me there is something wrong.

Let's assume that naming conventions are kept by developers (as they usually really are).

Good-code principles (from my point of view as a code-reviewer)

  • Make it readable
  • Minimize the amount of code
  • Reuse the code as much as possible (private methods, helper classes) - but remember here that sometimes to much code reuse can violate rule 1 - 'Make it readable' (The same situation is with database normalisation)

Code review principles

  • Do not be afraid of the code review
  • Review your own code to refactor it
  • Share your knowledge with your coleagues
  • Learn from your colleagues their code-snippets
  • Learn your helper classes and libraries you use


The most important principle of code review:

Review your own code to refactor it - when you write something (class, method or a poem) stop for a while after doing some portion and review what you did. If you know your tools well (libraries, helper classes, etc.), you can always find something that can be done more readable and smaller. In this moment check if some portion of your code could not be moved into a helper class and reused by other developers.


Here goes a list of some technical advices I always give when I review a code written in .NET 2.0:

Use private methods extensively

From my point of view this one is the most important as it is the most forgotten technique. I always see code that beggs for refactoring and usage of private methods. How does developer work (I work in exactly the same way): D creates portion of code that does something. When there is need to do something similar, D copies the few lines and modifies it slightly (I call it Copy-Paste-Modify design pattern - the most powerfull one). So any action of copying more than one row in VS should trigger an alert in our minds. In such case we should create a private method that does that in a generic way.

I use private methods not only to reuse code but also to make it more readable. Do not be affraid to extract private method that will be called only once but it will simplify the master method.

Create Helper (Utility) classes

The second and quite similar to 'Use private methods' principle. Create your own (your team) helper (or utlility) classes. If you work a lot with DataSet create DataSetHelper. If you work with ADO create ADOUtility. Put there lots of static methods that help you write your code simpler and cleaner (and faster to write) and with less errors. Many errors will be fixed within the helper. And the most important: share the information about the helper you created with your team. You can discuss (design) shape of the helper and make it very powerfull. But first step: DO IT !!!!!

Use consts

I see hundreds of strings hardcoded in methods. Some of them should be there but those strings that control logic should be placed in consts as they always occur more than once in the code. Moreover it allows me to find all the places that depend on the logic (with 'Find all references' in VS)

Place this everywhere

When I see the blue keyword this everywhere I feel the code is clean. In my opinion the this keyword should be everywhere it can be placed. It makes easy to see whether I use a method variable or class field/property. It is also blue so in a portion of black code it allows to spot some points so I can analyse it faster (maybe my brain is blue-coded or something).

Use operator ??

The ?? operator is almost forgotten by developers. It sometimes simplifies null-check logic and is even more powerfull with null-value objects. Null-value object is an object of the same type but representing null value of the object - such approach is rarelly used but I saw it few times.

Use nullable types

I see null check code many times in every application. Some of te code can be eliminated by usage of nullable types - e.g. int? which is an int that is nullable.

Use using(...) { ... } snippet

This one is simple and clean. It's called 'syntactic sugar' for try-catch-finally(dispose) but when I see it I know the code was deeply considered by developer. Unfortunatelly I do not see it very often. When you work with streams you should use it very often.

Use enumerator methods (IEnumetor with yield)

This one even I use rarely but it really helps simplify enumeration logic. It allows to create non-existing enumerator with almost no additional memory usage. If you have complicated foreach logic (which is quite often), try to use the iterator block.

Use anonymous delegates

When you need to do Transactions very often, create a method that takes delegate as argument - e.g. ADOUtility.Transactional(delegate() { ... } ). This approach allows you to avoid rewriting code that does something and is surrounded by some logic e.g. transactions support.

Use generic methods

This advice is usually connected with advice to create helper classes. It is quite often that creating a generic utility class makes it return object value (to keep it general). To avoid this you can create a generic method and compiler can do much more for you.

Use generic base classes

Generic base classes. When I develop something I usually find that classes are similar and I usually refactor them to create a base class. Doing that may cause that some of yor methods / properties are of object type. In such case you can alwyas make the base class a generic class that takes required types as arguments so you can still use intellisense and much more compiler support.

Use modified property accessors

This one is also rarely used. I always see that the class that contins lazy initialised property calls it via the field. So the code usually looks like that: this._MyProperty = "sth"; It is much cleaner when you use the same property for getting, setting and all the logic inside and outside the class. To make it working use modified property accessors - you can have a public property with public get method and with private set.

Try to not use one line if - (...) ? ... : ...

The one line if clause is in my opinion too often used. I do not encourage you to drop it completeley but to use it with care. It can simplify my logic a bit but only in simple cases it is readable. If the case is not the simplest one - use the if (...) { ... } construct instead.

Do not use ! operator

This one is the most controversial so I left it at the end. I used to use the ! operator very often but then I saw few times errors that it generates. The proberm with the ! operator is that it is very small and one can easily miss it in an if clause. There is no problem with the operator for the code owner but the code must be ussually maintained by few people. I saw errors that occurred after some other error fixing caused by the fact that the developer who fixed the bug did not notice the ! operator.

Posted on Sunday, September 16, 2007 9:38 AM | Back to top

Comments on this post: Code Review - from my point of view

# re: Code Review - from my point of view
Requesting Gravatar...
Good Article
Left by u on Sep 02, 2011 6:40 PM

Your comment:
 (will show your gravatar)

Copyright © Marcin Celej | Powered by: