This is the second in a series of posts about my thoughts on my redesign of the DX_SourceOutliner Visual Studio DXCore-based plugin that started here.

Observed Theme 3: Decision Logic is (Mostly) Driven by User Options

As mentioned, one of the very serious growing problems with the present design of the DX_SourceOutliner is that of decentralized decision logic.  Even with INodeProcessors and ITreeProcessors, this problem will still remain (to a degree).  Even though the decision logic will be neatly encapsulated in each implementation of these interfaces, since there will be many of these classes, decisions about ‘what icon should be assigned?’, ‘which nodes should be visible?’, etc. may be scattered all over the place still.

This can be seen in the following (hypothetical) implementation of ToolTipNodeProcessor…

  1: public class ToolTipNodeProcessor : INodeProcessor

  2: {

  3: 	TreeNode ProcessNode(TreeNode node)

  4: 	{

  5: 		if (showReturnTypeInTooltips)

  6: 		{

  7: 			node.ToolTipText = string.Format("{0} : {1}", node.Tag.Element.ReturnType, node.Tag.Element.Name);

  8: 		}

  9: 		else

 10: 		{

 11: 			node.ToolTipText = node.Tag.Element.Name;

 12: 		}

 13:		return node;

 14: 	}

 15: }

In this example, the ToolTipNodeProcessor is still doing way too much work for my tastes.  In addition to constructing and assigning the tooltip text to the node, its also encapsulating (in a bad way that we should probably refer to as obfuscating instead of encapsulating!) the decision logic about which of two choices for tooltip text formatting and content is being made within the guts of the ToolTipNodeProcessor class.

A better approach would probably be to increase specialization of types in the code and introduce two completely different classes, ReturnTypeToolTipNodeProcessor (that would include the return type in the tooltip) and UnadornedToolTipNodeProcessor (that wouldn’t bother to include the return type in the tooltip text.  Following is an idea about how this might work…

  1: public class ReturnTypeToolTipNodeProcessor : INodeProcessor

  2: {

  3: 	TreeNode ProcessNode(TreeNode node)

  4: 	{

  5: 		node.ToolTipText = string.Format("{0} : {1}", node.Tag.Element.ReturnType, node.Tag.Element.Name);

  6: 		return node;

  7: 	}

  8: }

  9: 

 10: public class UnadornedToolTipNodeProcessor : INodeProcessor

 11: {

 12: 	TreeNode ProcessNode(TreeNode node)

 13: 	{

 14: 		node.ToolTipText = node.Tag.Element.Name;

 15: 		return node;

 16: 	}

 17: }

As two separate classes, each of them has a much simpler structure, devoid of the potential bug-inducing ‘if’ statement that cluttered the middle of the initial ToolTipNodeProcessor class.  Take note that this hypothetical use-case is intentionally simplistic to get the design pattern point across; very little of the logic that’s in the existing solution is quite this basic.

Any time you have a giant ‘if’ in the middle of the method that is your class’ primary raison-d’etre, its at least worth considering (as I have here) that you might have an opportunity for specialization in your object model.  As developers, we are taught to always be on the watch for opportunities to introduce higher-level abstractions by seeking commonality in our class designs.  But the reverse is often equally beneficial: be on the lookout for the opportunity to create ever-more-specific refinements to your object model as well as it can simplify code, reduce bugs, and improve cohesion just as much as introducing abstractions in the other direction can help.

But now I have a brand new problem to solve: what’s responsible for deciding which of these two specialized node formatter classes to apply to the tree?

Enter the NodeProcessorSelector

This points to the need for a new role in the system, something I will tentatively call the NodeProcessorSelector.  This role needs to be able to logically determine which one or more INodeProcessors are appropriate for any given situation.  This decision might be based on a user-selected option (e.g., do or don’t display return values in tooltips) or other environmental factors (e.g., I’m in a VB.NET source file vs. a C# source file).  It needs to consume some construct that encapsulates all these variables, interrogate it, and make decisions based on it about which INodeProcessors should be applied to the nodes in the tree.

Its interface contract might look something like this…

publc interface INodeProcessorSelector

{

	IEnumerable<INodeProcesor> GetProcessors(NodeOptions options);

}

It would take in an object I’ll call NodeOptions and return a collection of appropriate INodeProcessors based on its ability to puzzle its way through the various settings encapsulated by the NodeOptions object.  This way, the logic about which ways to process the nodes is cleanly separated from the logic about how to process each node, leading to a coherent separation of concerns in the object model.

Further, just as with the other abstractions I’m considering introducing into the architecture, its very obvious where the extensibility points are for the (future) behavior of the application: when the logic about how to select one NodeProcessor vs. another changes, that change is encapsulated in the NodeProcessorSelector class and the remainder the application is shielded from the effects of any changes to the internal logic of this class.

There could even be multiple NodeProcessorSelectors for different contexts, in response to different user preferences, etc. leading to even more extensibility (e.g., the MutiDocumentNodeProcessorSelector, the SingleDocumentNodeProcessorSelector, etc.)

What Sets the NodeOptions Values?

So how do  the values get set on the NodeOptions object?  These come from a combination of the user-interface settings and user options for the application.  As it exists now, there are many lines of code in DX_SourceOutliner that read something along the lines of…

  1: if (chkShowMultipleDocumentsInTree.checked)

  2: {

  3: 	tree = RenderTreeWithMultipleDocuments();

  4: 	tree.Refresh();

  5: }

This represents an incredibly tight coupling between the UI layer and the tree rendering layer which means that high-order decision logic in the application is intimately entangled with the choice of how to render the tree to the user.  At best this is a maintenance challenge and at worst its an opportunity for hard-to-isolate logic bugs.  By transposing user-selections from the UI into their logical equivalents, the NodeOptions object can neatly help bridge the gap between the user interface and the underlying tree-composition and formatting engine.

Using the NodeOptions object, the above code could easily be restructured into something much cleaner and easier to maintain like…

  1: var options = new NodeOptions()

  2: 	{

  3: 		RenderMultipleDocuments = chkRenderMultipleDocuments.Checked,

  4: 		ShowMethods = chkShowMethods.Checked,

  5: 		ShowClasses = chkShowClasses.Checked

  6: 		//further property settings here as needed from the UI

  7: 	};

Essentially just a strongly-typed property-bag, NodeOptions will be used to pass the used-selected settings from the UI layer into the tree-construction and formatting layer in a way that will (hopefully!) result in not only a cleaner object design but also provide me a way to achieve something that I have been seeking for some time with this project: the ability to write reasonable unit tests for the code.

Design For Testability -> Better Design, period.

This whole redesign forces me to revisit yet another rule that I completely disregarded in v1 of the DX_SourceOutliner: the notion that “good OO software design is inherently testable and bad OO software design rarely is”.  When I talk to people about introducing unit testing into their toolset, whether its in the context of so-called “test-first” (or TDD) or in the context of more traditional “test-after” unit-testing approaches, one of the most common refrains I hear is “the need to test something is dictating the way I structure my object model”.  This makes people resistant to beginning unit tests in many cases as they feel that ‘testability’ is driving the design of their solution.

I’ve never subscribed to this stance at all because in my own experience highly-cohesive, loosely-coupled, narrow-focused object models that adhere to good OO principles are almost always inherently testable without much if any additional effort on your part to make them so.  The statement “I cannot find a way to test this class” is usually only true if you also finish the statement with “…because I violated several core tenets of good OO design.”  Lack of testability (whether you actually write the tests or not is another matter) should almost always be considered a code-smell in my experience.

In nearly every situation (this one included) when you undertake the effort to improve the class design it ‘becomes’ testable and if you undertake the effort to increase testability, the object model improves.  Both of these lead to higher-quality, more maintainable, readable code which is a win-win all around as far as I’m concerned.  Let’s set aside for a moment whether its even a legitimate complaint that ‘testability’ is impacting your design – I personally feel strongly that testability is a worthy design influencer and has a place at the table of competing design goals right alongside performance, usability, and other factors.

But in the v1 codebase of the DX_SourceOutliner, I blissfully ignored this lack-of-testability code-smell, even as I ploughed ahead to get the code working.  Like all good practices, you ignore them at your own peril.  In v1 I did so out of laziness and its come home to roost now in the form of my need to redesign the entire structure of the application in order to improve its extensibility and maintainability.  An extremely helpful side-effect of my going back to the codebase and doing this is that testability will increase substantially as a side-effect of this effort.

One More Observed Theme

There’s still one more remaining core theme for the DX_SourceOutliner that I have observed from reviewing the existing codebase, but more on that in the next post.