Nocturn vision

A friend of mine asked me about some tips ‘n tricks to apply when performing a code review on an application. It got me thinking and I had a few things to say, especially about NDepend, so why not sum them up in a post?

The context of this post:
You’ve got an application that is very important to you, but has some issues. You can’t get a stable version, you’re having performance problems, you’re getting lots of complaints, …….

Warnings:
I’m not going to get into details about how to get started with NDepend. My goal is to show you how I would use it to perform a code review. If you need some help to get you started, there are some great resources to look at.
I’m not saying the below order of things-to-do is the right order. I just randomly wrote down what I was thinking. Be sure to let me know what you would do differently, or what you would add, and don’t forget to specify why !

Tools that can help during code-reviews

There are several ways to do a code review, there also are several tools that aid to the analysis of existing code. NDepend, is one of the tools that will help you the most during a code review. As I’ve already said some other times, NDepend does code analysis on .NET applications.

Another great tool for static code analysis, is FxCop. It comes with a default set of rules that each developer should apply. You can even define your own rules, filter the results you get, and so on… If you take a quick look at both, you’ll think they do the same, but you’re wrong. They’ve got different mentalities. FxCop is all about rules applied to code, while NDepend, is in first instance, about metrics (86 of them!) on which you can run queries to get a better picture of what you’re analyzing. NDepend also offers the possiblity to generate warnings whenever some metrics go over a limit you don’t want to cross.

It’s not my intention to do an NDepend vs FxCop comparison-battle, so I’m just going to stop here. For a code review, I’d prefer NDepend because you’ve got more options to really understand the whole code base, period.

First things first

Define and understand what the product is for. What kind of application are you reviewing? Is it an application that needs complex calculations or has complex business rules? Do you need to be saving lots of data? Is performance an issue?

If you need to do a code review on an existing application, that’s because there are some problems with it. No-one invests in a full review of a working application that can take some days just to know how good the code is. 

What are the problems with it now? What’s the goal of this code review? Is it just to pin-point what should be refactored to improve the code-base, or are we talking about possible rewrites?  Are there bugs, that when fixed, just create other bugs? Is the code hard to change? Is it hard to maintain? Is it hard to understand?

Overall approach of the project

Are we working with an object-oriented domain model here? Or does the project use data-centric or data-driven approach? Depending on the type of the project, this can be either positive or negative, so it’s important to note.

One of the most common scenario’s in a data-centric approach (especially if the app is a few years old) is the use of DataSets. Using CQL, you can write a query that checks if typed DataSets exist in the project:

-- Look for typed datasets
SELECT TYPES WHERE DEPTHOFDERIVEFROM "System.Data.DataSet" == 1
-- Look for the use of datasets
SELECT TYPES WHERE DEPTH OF CREATEA "System.Data.DataSet" == 1

If you’re using objects instead of datasets, still you can’t be sure you’re dealing with an OO domain model. You’ll need to investigate the app some more.

What about layering?

How’s the application’s architecture? The first and most obvious thing to do is to take a look at the layering within the application.
If you just run the analysis, you’ll get the number of assemblies in the report.
Maybe, the fact you don’t have physical layering, is a good thing .

An important part of a code review, is checking how physical or logical layers use each other. In other words, you should look for dependency cycles. Visual Studio won’t even let you create a dependency cycle beteen assemblies, but you can seriously mess things up between namespaces.

Some basic CQL examples for physical layering:

SELECT TYPES WHERE IsDirectlyUsing "MyAssembly.Entities.Customer"

SELECT NAMESPACES
WHERE IsUsing "MyAssembly.Entities.Customer"
ORDER BY DepthOfIsUsing

-- Don't use DataLayer from Application layer
SELECT NAMESPACES
WHERE IsDirectlyUsing "DataLayer"
AND NameIs "Application"

Suppose you don’t have physical layering. Still, you don’t want your “Application.UserInterface” namespace to be using “DataLayer.CustomerDAL”, right? You can check it with this simple query:

SELECT TYPES FROM "Application.UserInterface"
WHERE IsDirectlyUsing "DataLayer.CustomerDAL"

You can explore the dependencies between your namespaces and types, using the dependency matrix and the dependency graph within NDepend. This needs some getting used to, at least, for me it did.

You can use the dependency matrix to track down dependency cyles. But you can also use a query to get started:

SELECT NAMESPACES WHERE ContainsTypeDependencyCycle

When I’m looking for dependency cycles, I start of with the dependency matrix, pinpoint the dependency (follow the red rectangulars), and finish off with CQL queries and dependency graphs. If you just write some “who is directly using me”-queries, you’ll soon get to see what types are causing the dependencies. If you want to see it visually, you can also generate a dependency graph of the dependency cycle to immediately see where it’s going wrong. That’s just a great feature!

Code quality and code health

After running the analysis, you’ll get an HTML report, that contains tons of information. When you’re done digesting that (which will be an overall view on the application), you can start to narrow your view, look at assemblies in isolation and look how it works internally. Narrow your scope with CQL-queries, or within the class browser (by clicking right on types).

NDepend has a lot of queries it runs during analysis, here are my favorites for code-health:

SELECT METHODS WHERE CyclomaticComplexity > 20
ORDER BY CyclomaticComplexity DESC

This query looks for methods that have complex if-while-for-foreach-case structures. They are usually in need of some refactoring-love, and in some cases even a design pattern can help (Strategy, Specification, …).

SELECT METHODS WHERE NbParameters > 5
ORDER BY NbParameters DESC

You can review the results of this query, and maybe even apply one of Kent Beck’s implementation patterns: Parameter Object. It states that if a group of parameters is passed together to many methods, consider making an object whose fields are those parameters and passing the object instead.

Here are some more queries to pinpoint refactoring-needed-code:

SELECT METHODS WHERE CouldBePrivate 

SELECT TOP 10 METHODS WHERE NbVariables > 15
ORDER BY NbVariables DESC 

SELECT TOP 10 METHODS WHERE NbLinesOfCode > 30
ORDER BY NbLinesOfCode DESC

There are some queries that can help you check if the developers respected the most important design principles:

Single Responsibility principle

SELECT TYPES WHERE TypeCe > 50 ORDER BY TypeCe DESC

Interface Seggregation Principle

SELECT TYPES WHERE IsInterface AND NbMethods > 15

YAGNI

SELECT METHODS WHERE
   MethodCa == 0
   AND !IsPublic
   AND !IsEntryPoint
   AND !IsExplicitInterfaceImpl
   AND !IsClassConstructor
   AND !IsFinalizer

I can’t really find a way to query for LSP, OCP or DIP violations, you can’t have it all folks!

Testing

NDepend makes it very easy to check code coverage (both NCover and VSTS coverage files are supported). Well, I already mentioned once, that code coverage doesn’t alway give a correct view on the quality of your tests. Still, you could choose to cover types, methods that are complex. This is a default CQL query that helps you identify uncovered complex methods:

SELECT METHODS WHERE
(
   NbILInstructions > 200
   OR ILCyclomaticComplexity > 50
   OR ILNestingDepth > 4
   OR NbParameters > 5
   OR NbVariables > 8
   OR NbOverloads > 6
)
AND !PercentageCoverage < 100

In older applications, you’ll be lucky if you even find tests. Personally, I think starting to write tests for existing code that hasn’t been tested at all, is a bit useless. In most cases, the code won’t even be testable. What I do encourage, is to write tests for the code refactored.

Performance

If you need to to some profiling to find out why you’re having performance issues, I would encourage the use of a profiler such as JetBrain’s dotTrace. If you’re just looking at NDepend’s predefined CQL-queries for performance, you’ll get queries for boxing, unboxing, big instance sizes, and so on. But they are not going to give you enough insight.

For example, if you’re using typed datasets, be sure to expect some exclamations for these queries. Typed datasets do nothing but boxing and unboxing whenever you do a get or set operation. On top of that, in most cases, they have a very big instance type, so instantiating them is very expensive. If you’re using them responsibly, this will not be the cause of your perf-issues. That’s why you should use a profiler instead.

Roundup

NDepend is a great tool, I think it’s the best on the market right now. What I’ve covered here are just basics and many of the queries I showed you are predefined and executed with each analysis. Once you start experimenting more with the CQL language, the metrics out our disposal, and with the dependency matrix, you’ll get more and more insight in the codebase you’re analyzing.

I think it’s also a great tool to check how your own development skills are! I’ve been in the situation several times that I would wish for a developer to review my work, tell me what’s good, but especially what’s not good – what’s wrong with it and what I can do about it -. If this is just not possible, analyze your code with NDepend, and I’m sure you’ll find some mistakes that you won’t ever repeat in the future.

My post-title was getting too long, so I’ll just finish it here:

… and what you should do about it!

Actually, this is a bit of a follow up to post to my post about design principles. I felt I had something more to say about YAGNI in combination with software factories. Not very long ago, I talked about a software factory I layed my hands on.
Now, I’ve had the chance to mess around with it a little more.

Imagine starting a new project with a software factory. It generates a lot of code. Some of it usefull, some of it not very.
I’m asking two questions:
1) What code is actually usefull? => It depends on the case
2) What code is not usefull at all? => It depends on the case

According to YAGNI, we shouldn’t be writing code that we’re not going to use. But in this case, a lot of code we’re not going to use, has already been generated by the software factory. So the software factory breaks the YAGNI principle.
I don’t find it abnormal that the code generated by a SF breaks the YAGNI principle, but that doesn’t mean it’s OK to keep the code breaking YAGNI when using a SF!

Why does a SF break the principle?

A software factory, solves a common problem. But as we all know, every problem domain, has exceptions. A software factory will only be able to solve that what’s -generally- the same over all cases, thus the solution offered by the SF, will not be entirely correct in all cases.
Since we’re not starting from scratch with the codebase, the developer can’t avoid breaking the principle, since it’s the SF that breaks it. But as I said, that doesn’t mean it’s ok. While in cases we don’t use a SF, it’s the developer’s responsibility not to break the principle, now it’s the developer’s responsibility to clean up the mess the SF made.

Time to refactor the software factory?

If the SF is generating code that breaks YAGNI, doesn’t that mean it’s time to refactor? Well, yes and no. The SF is nothing more than an existing codebase, just like any other one. So it evolves with time, and like in every project, it also needs it’s refactoring love once in a while. We all know refactoring is a good thing.
During your refactoring-time, you could take into account what code to add, but most importantly since I’m talking about YAGNI here, what code to remove.
But that’s not an easy decision, since that code is usefull in one project, but not in the other one. The only code you can delete without thinking about it twice, is the code that’s just -never- used.
The code that deserves some serious brainstorming, headaches and analysis, is the code that is used in less than 80% of the cases.

The SF guys might be thinking: 80%??? Then you’re taking away 20% of value? And I’ll say, no, I’m not at all. I’m taking away the code, that is an added value for 20% of your projects, but that is an added misvalue (i had to give the kid a name) for the other 80% of the projects! I must say the developers that created this SF, thought about YAGNI carefully. If you only have about 20% that is YAGNI-code in code generated by a SF, I think we can say we’ve got ourselves a powerfull thingie here.

But still, you’ve got that overhead. That 20% of other code that’s only used in special cases.
You can’t delete it, still it bothers the hell out of you. And more important, it breaks the YAGNI principle! It makes your code base too big for nothing, and sometimes very confusing to work with.

Things I think should not be generated in the SF

- method overrides that only call your base implemenation: override them when you need them, typing “identifier override methodName” is not that hard, is it?
- tests that aren’t testing anything but prepare your mocks and variables: you could still have this partly handled by the SF, by creating a recipe for them

I’m sure I can add things, but these are the most obvious ones I’ve seen until now.

So what do we do about it?

Well, in my suggestion, I would just leave it in there… But not until the end of time!
You have to pay off your technical debt, and in this case it means you have to remove all YAGNI code! Plan some time to do this after you’ve finished a user story. If this seems to be too often, and you’re not removing hardly any code, you could choose to do this after each iteration. But don’t prolong it more than that, or you’ll end up not doing it at all, since it will take too much time to do it all in one go.

You’ll think, but what if I need that code in another user story, or an upcoming iteration? That’s where you’re breaking the principle. No excuses. After you’ve finished a user story or an interation, and you haven’t used the code, the chance you will be using it all, decreases a lot.
If you need it anyway in the end, just add it manually, it won’t kill you, I promise.

Important note!

If for some reason you need to regenerate your code (executing DSL’s for example), you should review all generated code for unused code again! And not after the iteration. I think in this case, it’s something you should do instantly. Do it at the same moment you regenerate your code. Consider it a part of regenerating your code. If you notice you’re regenerating a lot of code in different places while your change is small, then I would advice to take a look at your SF. It should offer you a way to regenerate parts of code, without affecting other ones.

How to identify YAGNI code?

Resharper already makes it easy to see YAGNI code (it will look gray) such as:
- unused overrides
- unused private methods
- unused parameters
- redundant code

But that won’t get you all the way through the cleanup. ReSharper only detects these redundancies locally (I’m hearing in a next version this option will be available solution wide), and it also gets harder with unused types. There are other tools that help you out with those problems.

FxCop is a great tool offered by Microsoft to perform static code analysis on compiled assemblies. On the other hand, -and I personally think this is the most powerfull code analysis tool on the market-, we’ve got NDepend. It analyzes your code, and let’s you examine dependencies, complexity, and  a bunch of other stuff using code metrics, graphs, other visual representations and -last but certainly not least-, CQL queries. NDepend already executes some CQL queries during the analysis, but afterwards you can even execute more specific CQL queries to analyze your codebase.

In our case, we’re looking for unused code. This CQL query is included in the analysis, so it’s veeeery easy! Just take a look at the results, and refactor them away.

For your information, here are the basic CQL queries NDepend uses for detection of dead code:

WARN IF Count > 0 IN SELECT TOP 10 TYPES WHERE
TypeCa == 0 AND     // Ca=0 -> No Afferent Coupling -> The type is not used in the context of this application.
!IsPublic AND       // Public types might be used by client applications of your assemblies.
!NameIs "Program"   // Generally, types named Program contain a Main() entry-point method and this condition avoid to consider such type as unused code.

WARN IF Count > 0 IN SELECT TOP 10 FIELDS WHERE
FieldCa == 0 AND  // Ca=0 -> No Afferent Coupling -> The field is not used in the context of this application.
!IsPublic AND     // Although not recommended, public fields might be used by client applications of your assemblies.
!IsLiteral AND    // The IL code never explicitely uses literal fields.
!IsEnumValue AND  // The IL code never explicitely uses enumeration value.
!NameIs "value__" // Field named 'value__' are relative to enumerations and the IL code never explicitely uses them.

WARN IF Count > 0 IN SELECT TOP 10 METHODS WHERE
MethodCa == 0 AND            // Ca=0 -> No Afferent Coupling -> The method is not used in the context of this application.
!IsPublic AND                // Public methods might be used by client applications of your assemblies.
!IsEntryPoint AND            // Main() method is not used by-design.
!IsExplicitInterfaceImpl AND // The IL code never explicitely calls explicit interface methods implementation.
!IsClassConstructor AND      // The IL code never explicitely calls class constructors.
!IsFinalizer                 // The IL code never explicitely calls finalizers.

If you haven’t looked at NDepend yet, I strongly advice you to do so!

Then, you’ll have a non-YAGNI codebase again.