SheepAop Code Review with NDepend (Part I)

Now that SheepAop has reached its first milestone, I think it’s time to stop for a moment and review the code to discover potential code-smells before I engage any further development.
I’ll use NDepend for our code-review exercise today, and I’ll be doing it live while I’m reviewing the code, refactor, and write it down on this post on every step as I go along.

Dependency Graph

clip_image002[4]

The very first thing I did was to check the dependency graph to get an overall understanding about the structure of the code. To my surprise, the dependencies did not immediately look quite as obvious as the picture I had in my head.

A large part of the graph still fits very well with what I expected:

  1. There are some helper-classes and exceptions down at the bottom, which contain general-purpose classes that help streamline our contact with the outside world (.net collections, mono-cecil, etc). These helpers are common and should be completely agnostic to anything SheepAop, which is confirmed by this graph: there’s no line coming out from these modules.
  2. SheepAop.Aspects contains a set of out-of-the-box sample implementations of SheepAop aspects. They’re not part of the framework, so I don’t expect to see anything SheepAop to reference it. The graph agrees.
  3. SAQL is a DSL layer on top of SheepAop API. The core machinery of SheepAop should be completely agnostic of SAQL. The graph looks fine: all Saql namespaces appear to be left alone by the rest of the town.
  4. SheepAop is designed so that making new Advice implementations should be easy because it’s the one part in SheepAop that will change (and be added) the most. For that reason, all advice classes should be external to the core so that it’s easy to plug new ones. The graph seems to agree with that. There are currently only 2 advices in SheepAop: AroundAdvice and LifecycleAdvice, and the graph shows no arrow pointing to that direction. (Except the unexpected arrow from Pointcuts to AroundAdvising, which we’ll explore below).

Potential Problems

There are, however, few things that I was not categorically pleased. I’ll list them all as a note for now, even though I’m not sure whether or not they turn out to be code-smells. But we will investigate each one of them later on.

  1. I was surprised to see how much SheepAop.Compile namespace assumes such a central role in the whole architecture. Its responsibility is supposed to be limited to the compilation part of SheepAop. It should depend on other SheepAop components to do this task, but conversely the other components should NOT have much interest in the compilation business. The graph, however, shows that almost every part of SheepAop depends on the compiler, which doesn’t seem right.
  2. As mentioned above (in point#4), SheepAop Advice classes should be pluggable modules that adding new ones should be cheap and easy. I’m quite curious about why SheepAop.Pointcuts namespace has any hard reference to AroundAdvice.
  3. I also did not expect to see SheepAop.Core to depend on the Runtime component and Attributes. Attributes is supposed to provide users with a friendly interface to interact with the core SheepAop API. What’re the attributes possibly doing that makes them so critical even the core engine has to depend on it?
  4. Like Advices, Lifecycles needs to be a pluggable thing so that it’s easy to add new ones (I’m still developing some as we speak), as well as for users to make their own custom Lifecycles to be plugged to SheepAop. I was hoping not to see any hard reference from the Core to Lifecycles if I can help it. We’ll see what we can do.
  5. (After the first refactoring below) We have cyclic dependencies between Core<->Pointcut. We’ll investigate this. Probably Pointcut should really be part of the Core, considering how central the role it plays in SheepAop.

Investigation

Let’s now investigate each of them. I’ll start from the first one.

1. Everything -> SheepAop.Compiler

There are currently several dependencies toward the compiler which we want to get rid of. I’ll start with the most worrying one: the circular dependency between the Compiler and the Core.

clip_image003[4]

The one I need to look out for is the reference INTO the Compiler (greens and blacks). The answer seems quite obvious here: AdviceBase and AspectDefinition are defined as Compiler components. They are not. They have little to do with compilation. Let’s try moving these classes into the Core namespace. If they are indeed part of the Compiler, there will appear even more dependencies toward the Compiler coming from these classes. I’ll also need to bring along IAdvice and IWeaver since they are depended upon. So let’s try and refactor this.

clip_image005[4]

Great! The dependencies are now heading to one direction. All the unit-tests still pass. So this one is sorted. Let’s take a look on our next dependency on the Compiler.

Wait, they’re all gone!

clip_image006[4]

No more circular dependency on the Compile components (orange). It turns out that those 4 classes we just refactored were the culprit of all the heavy dependencies on the Compiler. As shown by the green blocks, the only components that now depend on our Compiler are our 2 Advices: AroundAdvice and LifecycleAdvice. That sounds perfectly correct. And the picture is starting to take its shape.

2. Pointcuts -> AroundAdvising

We want all Advices (AroundAdvice in this case) to be decoupled from the framework because it’s one part in SheepAop that will change (and added) the most, so it has to be easy to plug new Advices.

Same deal, I click on the line between Pointcuts and AroundAdvising to view the matrix. (It’s quite a tall picture so I cut out the irrelevant bits).

clip_image008

The green squares are the ones that shouldn’t be there. I wonder what they are. Let’s check the first one: PropertyPointcut.

clip_image010

That’s a method I found in the class. That’s a leftover method I inherit from the old version of SheepAop infrastructure (when Advice was still tightly coupled to the system). I don’t think they still contribute to the new SheepAop infrastructure. I checked the other green squares and they all seem to have the same garbage leftovers. Let’s get rid of them and run the unit-tests to make sure it doesn’t break anything.

clip_image011

Great, all the greens are cleared. All unit-tests still pass.

clip_image013

Now AroundAdvice is completely decoupled from SheepAop. The graph (centered on AroundAdvising) shows no green, meaning that there’s nothing in the whole SheepAop that depends on it (except our public Attributes). So this has been another success.

3.A SheepAop.Core <-> SheepAop.Runtime

There are several components that SheepAop.Core is dependent on, which it shouldn’t. Let’s start with the Runtime namespace. SheepAop Core should not depend on its Runtime components.

clip_image015

The green ones should not be there. It seems that they are all pointing to 2 method delegates (AdviceInvoker and AdviceCallback at the top), all of which are coming from Joinpoint classes. It makes me feel that those 2 delegates are integral parts of SheepAop Joinpoint system. Maybe I should put them together with the Joinpoint. In fact, I start to feel that these objects warrant their own separate namespace ( “Joinpoint”, instead of piggybacking the “Core” namespace). But first thing first.

clip_image017

Great. Now let’s split all those Joinpoint classes to a separate namespace, and hope that the resulting namespaces won’t cause cyclic dependencies.

clip_image019

Wow, this is unexpected. Not only that Core and Joinpoint did not end up in a cyclic dependency, there is in fact no dependency at all between them! They are both unrelated. It really was, after all, a good decision to split them to separate namespaces. But now it makes me wonder about the name. What’s with the name “SheepAop.Core.Joinpoint” if it’s not related to the Core? Since Runtime is the biggest dependent of this namespace, I think I should merge it with Runtime. After all, a join-point is a SheepAop representation model to describe runtime snapshots. Ironically, it turns out that the 2 delegates we pulled out of Runtime namespace were actually at the right place after all.

clip_image021

OK, we’re good. Let’s move on.

3.B SheepAop.Core <-> SheepAop.Attributes

As explained at the start of this post, Attributes is a facade API for users to interact with SheepAop framework, and it should sit right at the edge, together with SAQL. So why does the Core depend on it?

image
Aha, a black one. SingletonAttribute extends LifecycleAttributeBase, but LifecycleAttributeBase has a static CurrentInstance that defaults to SingletonAttribute. A cyclic.

image

Generally I have no problem with this kind of cyclic dependency because it does not indicate a code smell. But in this particular case, LifecycleAttributeBase needs to be refactored anyway into an ILifecycleProvider interface which will fit better as a Core component. In general, attributes are intended to be used as user API. Doesn’t feel right to see it as a part of the Core engine. So yeah, I’ll refactor this, and hope that it will also solve the circular dependency.

image

It does. That was quite a big refactoring, but all the unit-tests still pass, so we’re good. We’re now on a much better state with this refactoring.
As shown by the green blocks, the only component that knows anything about SheepAop Attributes (orange) is now only the Compiler, which sounds fantastically correct. (SheepAop.Aspects, as mentioned earlier, is just a sample implementation of SheepAop. So just ignore it, it’s not part of the framework).

Notice that our refactoring has also cleared the cyclic dependency between the Core and Lifecycle, so we just unconsciously solved the first half of task#4.

4.A Sheep.Core.Lifecycle <-> Sheep.Core

(Done)

4.B Sheep.Aop.Lifecycle <-> SheepAop.Runtime

To reiterate, we want Lifecycle to be decoupled from the framework so that it’s easy to introduce new ones (which are being developed as we speak), and for users to add their custom ones.

image

Ah this one is easy. IMayHaveAspect is a very specific spare-part of the PerThis lifecycle, so it should really belong in the Lifecycle namespace. So let’s move it.

image

Ah looks much better now.

5. SheepAop.Core <-> SheepAop.Pointcuts

I have a strong feeling that Pointcut should be merged into the Core. But let’s see.

image

Ah it looks like the Core only knows Pointcut by IPointcut interface and PointcutBase. I think I can move IPointcut to be part of the Core, and refactor the reference to PointcutBase to use the interface instead. (It’s generally a good idea to program against interfaces anyway).

image

Sweet. The cyclic traffic is gone, and the dependency becomes unidirectional: SheepAop.Pointcut –> SheepAop.Core, which feels quite right. So this is our final result:

image

Now we can see no more direct cyclic dependency in our code. But I can’t convincingly tell whether there is any indirect circular dependencies in the graph.
The graph is messy. Now that the relationships between the components have changed substantially, I think we should rearrange our graph to better reflect the new shape of our architecture. (I’ll also hide helper classes and exceptions from the graph).

image

The modules are now arranged so that all dependencies flow from top to bottom. The structure of the architecture is now very visible from the graph. As you can see, all arrows are pointing downwards. Or in simple terms: there is no possible circular dependency between our modules, directly or indirectly.

Next Step

I think that’s it for today, I’m calling it a night. We have achieved quite a good result so far. Tomorrow (or whenever I got time), I will continue the review with NDepend Metrics and CQL code-inspections. I can see some warnings already on my NDepend panels indicating some problems detected by NDepend code-inspector. But I’m too tired for the day, so I’ll spare that for the next post.

All refactored code has been checked back in to the SheepAop repository. You can check it out if you want to explore the result of today’s refactoring.

Advertisements

3 thoughts on “SheepAop Code Review with NDepend (Part I)

    1. Cheers skolima. I use Windows Live Writer, which doesn’t really tell you what format it uploads your images as (not one that I noticed anyway), but I’ll make sure I check that the next time I write.
      I don’t like WLW to resize my text-images as well, but I can’t seem to find any way in WLW to output no resize options on image markups.
      In the past, I’ve had to edit every image markup on my post manually in WordPress to remove all resize options on them.
      In short, I’m in a desperate need for a better blog writer app.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s