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.
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:
- 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.
- 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.
- 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.
- 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).
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.
- 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.
- 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.
- 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?
- 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.
- (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.
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.
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.
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!
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).
The green squares are the ones that shouldn’t be there. I wonder what they are. Let’s check the first one: PropertyPointcut.
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.
Great, all the greens are cleared. All unit-tests still pass.
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.
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.
Great. Now let’s split all those Joinpoint classes to a separate namespace, and hope that the resulting namespaces won’t cause cyclic dependencies.
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.
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?
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.
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
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.
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.
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.
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).
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:
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).
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.
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.