Two Screencasts on How to Demystify Spaghetti Code

In my consultant career, no matter the kind of company I visited, from the tiny startup to the largest fortune 500 corporation, they all have in common to be entangled in spaghetti. Spaghetti means poorly structured code. Spaghetti means high maintenance and evolution cost. Spaghetti means frustration, friction and lack of motivation for everyone in the team. And often, spaghetti means project failure.

Mastering code structure means demystifying and getting rid of spaghetti code! With Filip Ekberg, we propose you to cover in-depth code dependencies management through two 30 minutes screencasts. The tool NDepend is used to shed light on the structure of real-world code bases.

The first screencast shows how to make the best of dependency graph and dependency matrix tooling to explore the structure of a .NET application. This includes detecting at a glance good patterns or smelly patterns, buried in the code. In this screencast, we also explain how to master interaction between code query LINQ generation and dependency graph and matrix. Doing so constitutes a very powerful way to generate all custom graph you really need to visualize, like custom call graphs and inheritance graphs.

The second screencast starts with focusing on dependency code rules. This is especially useful to write rules related to Object-Oriented tenets, like base class shouldn’t use derivatives, or to list instance method that could be made static. We then continue with rules concerned with layered architecture, like UI layer shouldn’t use the DAL layer or components dependency cycles detection. We finally explore entangled layering in the real-world, and explain how to get rid of spaghetti and achieve well structured code bases.

These screencats contain many original and useful haha hints that will improve the way you and your team manage your .NET code base dependencies.

Enjoy :)

Part I

Part II

Posted in .NET assemblies, .NET Framework, .NET Fx, Acyclic componentization, Code Dependency, Component, CQLinq, Cycle, DAG, Dependencies, Dependency Cycle, Dependency Graph, Dependency Matrix, graph of callers, Graph of Dependencies, Indirect Dependency, Pattern, Patterns | 1 Comment

Validating Architecture through LINQ Query

These days we are restructuring the NDepend code base to make it more suited to welcome future features implementation. Here is below the new architecture of the NDepend.UI assembly, made of around 50.000 lines of code, shown through a Dependency Structure Matrix:

As always, the dependency matrix naturally exhibits architectural patterns. Keep in mind that a blue cell means: the namespace in column is using the namespace in row. The number on a blue cell represents the number of members used (see the option in the matrix menu: Weight on Cells set to Direct: # members). This cell number is then a measure of the coupling between the two namespaces.

Here are architecture patterns emerging from this dependency matrix:

  • There is one Top namespace, that is using all namespaces (blue column #0)
  • There is one Base namespace, that is used by all namespaces (blue row #17)
  • The Panels namespaces don’t use each others (empty big red square, rows/columns [1-12])
  • The Shared namespaces don’t use each others (empty small red square, rows/columns [13-16])
  • The blue cells in the rectangle rows [13,16] x columns [1,12], represents the usage of shared features by the panels. For example, on row #15 we can see that the DependencyContextMenu feature is used by the Top, Graph and Matrix impl.

I found this NDepend.UI architecture pretty neat. Whether we will add a new panel or a new shared feature in the future, it will be inserted naturally in this structure. And this architecture is simple enough to get validated at a glance. But at a glance is not enough. What about defining custom rules that take care of validating this architecture and inform us automatically of any violation?

Each of this architecture pattern could be actually validated through a Code Query LINQ rule. Let’s go through 5 steps to write the rule Panels shouldn’t use each others through a LINQ query.

5 steps to write the rule: Panels shouldn’t use each others

Step 1: First, let’s match all panels namespaces. Here we are using the extension method WithNameWildcardMatch() to match the panels namespaces by name. We can see that some panels (Matrix, QueryEdit, Search…) are made of several sub-namespaces. The rule must be smart enough to both:

  • let sub-namespaces of a panel use each others,
  • forbid sub-namespaces of different panels use each others:

Step 2: Now let’s use the method IUser.IsUsing() to list the couples of [namespace user, namespace used]. Notice that to improve the query performance:

  • we are restraining the set of namespaces user only to the panels namespaces that are indeed using another panel namespace (with the extension method UsingAny() )
  • we are restraining the set of namespaces used only to the panels namespaces that are indeed used by another panel namespace (with the extension method UsedByAny() )
And indeed the query runs fast: 5 milli-seconds on a more than 130.000 lines of code code base! There are 45 couples of [namespace user, namespace used] listed:

Step 3: Now, we need to validate for each couple of [namespace user, namespace used], that both user/used namespaces belong to the same panel. For that we have to work on the namespace name string. Let’s first get rid of the prefix “NDepend.UI.Panels.” and obtain for each namespace what we call its sub-name:

Step 4: Let’s get the panel name from the namespace sub-name. For that we are using the ternary operator: if the namespace sub-name doens’t contain any dot, it is already the panel name. Else, the panel name is the sub-string until the first dot met in the sub-name:

Step 5: Et voila! We now just have to make sure that for each couple of [namespace user, namespace used] they have the same panel name! This is as simple as adding the where clause: where nUserPanel != nUsedPanel.

We then add the prefix warnif count > 0 to transform our code query into a code rule. Plus, we set the name of the rule to Panels shouldn’t use each others. And the rule is ready to be saved, and to be run continuously in Visual Studio and/or on the build machine.

Note that this rule gets compiled and executed in only 7 milli-seconds (see 7 ms on the screenshot). This means that a typical machine can run in one second more than 140 of such complex rules against a large code base!

And also, when new panels will be developed, this rule won’t have to be updated. It’ll just work!

Could it be easier or faster to validate the architecture of a big lump of code?

Btw, just a quick side remark: The fact that panels implementations are not using each other doesn’t mean that panel shouldn’t interact with each other. They actually do interact a lot with each others. But they do interact through using a mediator implementation that lives in the Base namespace. This mediator then calls the Top implementation (through inversion of control) that takes care of dispatching any interaction to the right panel.

Posted in C#, Code Dependency, Code Query, Code Rule, code structure, Code visualization, CQLinq, Dependency Matrix, Layer, LINQ, namespace, namespaces, NDepend, Pattern, Patterns, Performance | 2 Comments

Screencast: Inspecting Code Quality and Code Complexity

Recently I had a chance to record a screencast on Inspecting Code Quality and Code Complexity with Filip Ekberg. In this half-hour video we inspect the NUnit v2.5.8 code base quality and complexity with NDepend v4.0:

  • We first use the popular Cyclomatic Complexity code metric…
  • …we then refine our findings by focusing on code quality regression since NUnit v2.5.3.
  • …some more code metrics are then involved, including the code coverage ratio by tests, the C.R.A.P metric and the code coupling ratio.
We plan to do more screencast on other programming related topics. Your feedbacks and suggestions are welcomed.
Enjoy!

Posted in CC, Code Diff, Code metrics, Code Rule, Cyclomatic complexity | 1 Comment

Beware mixing List.Sort() and Thread.Abort()

After more than a decade of full-time development with .NET, I still discover surprising unfixed bugs in the heart of the framework. Today, while I was smoke-testing new stuff in the NDepend UI, I stumbled on an InvalidOperationException thrown by a call to List<T>.Sort() ?!

After 15 minutes of searching the cause of the bug, I found this unfixed bug on Microsoft Connect: List.Sort() throws InvalidOperationException (instead of re-throw ThreadAbortException) while thread is being aborted

In other words, if your application is using Thread.Abort() somehow, and that the abortable code is calling List<T>.Sort(), you are exposed to the risk of getting an InvalidOperationException instead of a ThreadAbortException.

I already hear that it is a good practice to avoid calling Thread.Abort(), but the fact is that if one wants a responsive UI, one needs some sort of abortable/timed-out background work, and at a point Thread.Abort() will have to be called for that. As long as calling Thread.Abort() is done in a safe way (see this Joe Duffy detailled post) we didn’t meet any problems so far. On millions of production runs logged, we cannot see any problem except … that List<T>.Sort() throws an InvalidOperationException with the frequency of around one on every 50K production runs. And today, this bug just popped to me in a reproductible way thanks to some refactoring I was doing.

To workaround this problem once for all, I enclosed all calls to any override of List<T>.Sort() in some extension methods in a dedicated class. These BugFreeSort() methods catch the InvalidOperationException. This is fine because anyway the thread is in a ThreadState.AbortRequested and the CLR will throw the ThreadAbortException at the end of the catch block. Also, attached to this class through an attribute, I declare a CQLinq code rule in the source code to make sure that all calls to List<T>.Sort() are made through one of these BugFreeSort() methods. Here is the code:

Here is the code rule declared in the code. Hopefully any .NET developer acquainted with the LINQ syntax, should be able to read this piece to code:

And here is a screenshot of the NDepend methods violating the code rule before refactoring:

We have almost 200 code rules declared in source code like this newly added one. They protect us from stumbling again and again in tricky identified scenarios. In other words with these rules we abide by the tenet: Fool me once, don’t fool me twice.

In a team context, doing so is especially useful since such rule is warning a developer when he’s doing something wrong. Such warning is actually a just-in-time automatic piece of education for newcomers developers, about how things must be done. Communication in the team is enhanced, the overall friction is decreased, and I found this pretty cool :)

Posted in .NET Framework, .NET Fx, C#, Code Query, Code Rule, CQLinq | 6 Comments

The consequence of a.NET Framework API design flaw

Last week, a user reported a bug on NDepend (hopefully it happens in a very rare situation!). After mulling over a bit on the bug stack trace, it appears that the bug is the consequence of a call to ICollection<T>.Add() while the underlying collection is an array!

Here I felt bad, because really the bug is a consequence of a .NET Framework API design flaw. Or better said, to take our responsibility, the bug is the consequence of us, not providing a work around seriously enough on this  .NET Framework API design flaw. Why the hell some read-only collection implements the Add() method? It is even getting worse if considering the documentation of Array.Add() (as a IList<T> explicit implementation method) and ReadOnlyCollection.ICollection<T>.Add().

 This implementation always throws NotSupportedExceptionOUCH!

It appears that finally, a decade after .NET 1.0 has been release, 3 interfaces IReadOnlyCollection<T>IReadOnlyList<T>IReadOnlyDictionary<T> will be presented by the .NET v4.5. As commonly said, it is never too late to do things right, but in the case of an API with ascendant compatibility used by millions of developers world-wide, this proverb doesn’t fit that well.

The fact is that the brand new NDepend.API actually uses the interface ICollection<T> to return read-only lists. I wouldn’t like us or some users stumble on this API design flaw again! Hence a decision was taken to change the API policy, even though this will introduce breaking-change (the API is still young enough for that). First, I wrote the following CQLinq query to evaluate the problem:

Here is the result, 40 methods (or property getters) returns something that implement IEnumerable<T> and many of them returns a ICollection<T>.

So what the API should return instead of ICollection<T>? Knowing that NDepend is still compatible with VS2008 and hence is compiled with .NET 3.5, it cannot use the brand new .NET v4.5 IReadOnlyList<T> (but it will certainly use these in several years, when we will compile against .NET v4.5 and above).

  • Returning arrays instead of ICollection<T> could be an idea. The problem is that array is an implementation, and it is not read-only since the user can update each slot of the array.
  • I never really liked the class ReadOnlyCollection<T> that wraps a IList<T>. First it is a class and not an interface. Second it has the property getter Items that can returns the wrapped IList<T>, so it is not that read-only and not a good wrapper at all! Third it implements IList<T>, and all mutable members are not supported, so it can lead to the same kind of confusion we are now faced with.
  • Returning some IEnumerable<T> would be a better choice than the two above. It is not ideal through. While IEnumerable<T> conveys well the read-only idea, it has no Count nor an item access by index property. Hence it is not really suited to return a well-sized list. LINQ provides the adequate extension methods Count() and ElementAt(), but  IEnumerable<T> has a strong taste of sequence, while we’d really like to provide users with proper lists!

So we choose to provide our own interfaces IReadOnlyCollection<T> and IReadOnlyList<T>! Same as the ones released soon in .NET v4.5, but defined in another namespace NDepend.Helpers, this will discard most naming collisions for clients. Thanks to this choice, the API is as clean as possible for now, and the transition will be simplified once NDepend will be compiled for .NET v4.5 and above, sooner or later.

As more and more often, I found inspiration in a stackoverflow.com response here. Find below the whole interfaces declarations and implementations. This is not an ideal solution, since List<T> and Array of course doesn’t implement our interfaces, but I found it to be a good design compromise.

Notice the four factory extension methods, that make a clear distinction at creation time, between Wrapped and Cloned read-only collection. Notice as well that the Cloned factories, take any sequence as argument, while the Wrapped factories, need collections and lists as argument:

  • ToReadOnlyWrappedCollection(this ICollection<T>), 
  • ToReadOnlyClonedCollection(this IEnumerable<T>),
  • ToReadOnlyWrappedList(this IList<T>), 
  • ToReadOnlyClonedList(this IEnumerable<T>),

Also, we’ve added the IndexOf() extension method over our two interfaces, because we needed it, and because the .NET Framework doesn’t provide IndexOf(this IEnumerable<T>) because a sequence is not necessarily ordered (like hashset or dictionary).

And finally, I did a CQLinq rule to check that NDepend.API will never return anymore a ICollection<T>!

Posted in .NET Framework, API usage, breaking changes, Code Rule | 11 Comments