Friday, March 13, 2015

Assert.DoesNotThrow Does not Preserve Stack Traces

Just a short post about an issue with an NUnit feature.

It has an assertion called DoesNotThrow that takes a delegate function as an argument. If the delegate throws an exception then NUnit will throw its own exception. The method is great at pointing out exactly where the code under test is. However, if an exception does occur, NUnit reports the exception type, but looses the original stack trace. There have been enough occurrences of lost stack traces that I've stopped using it. Now I just comment the actual test line and let the exception bubble up to the test runner.

Wednesday, March 4, 2015

Make Classes not Enums

The main C# code base I work with at work is full of generic objects and methods written to use enums to drive behaviour. I'm not sure of the reason. The people who created it have all since moved on, so I don't know who to ask. All I can do is live with the giant switch statements and undo them into classes when I can. Why classes? Let's take a look.

The full example code base on which the gists are taken is available in https://github.com/claq2/MakeClassesNotEnums.

The (Evil) Enums Way


Say you have a service that creates different kinds of backup jobs. One of its arguments is an enum that indicates what kind of job to create. It returns only 1 kind of object, Job, that is not abstract. This object represents all the different possible kinds of jobs. A property, JobType, indicates whether it's for backing up files or a SQL database.

In this example, creating a job means calling a single CreateJob method that takes an enum argument that indicates the type. This might seem contrived, as in "who would build this", but it's easy for a UI to consume. Show the user a drop down with the job types and call the one method with the user's choice in the enum.

In object oriented programming there is something called the Single Responsibility Principle. It states that a class should have only 1 reason to change. Another way to put it is that a class should have exactly 1 concern. The enum approach violates this in at least 2 ways.

First, the Job class represents all possible job types. It needs properties for every job type. A change to any job type or the addition of a new one means changes to this class. Some properties apply to all types and some apply to only some types. How do upstream developers of the Job class know which properties to fill in for each type? They have to look at the documentation. I hope they don't miss one of the comments.

Second, the JobService has 3 concerns - determining which type of job to build, verifying the values and mapping input values to properties. If any of these aspects change this class must be updated. The more you need to change it, the more risk there is that you will introduce a bug. Especially if the switch has many clauses with pages of complex logic.

The cyclomatic complexity of this method is already 7, meaning you need at least 7 unit tests to completely cover it. This approach will only get harder to test fully and understand. There's already a little extra logic for Sql2014Jobs. How long before the logic becomes so tied together that you have to write tortured logic to get exactly what you want?

Think about what load and save methods in the JobService class will look like. Nearly the same - more switches with extra if statements for the special cases.

The (Righteous) Classes Way


There are 2 sources of cyclometric complexity in a system - the conditions that the business defines and those created by developers. You usually can't reduce the business's conditions, but you should be able to control the developer-created ones. The way you do this in an object oriented system is usually with classes and inheritance.

In this example one might wonder why the interface doesn't already know what kind of job it need to create in response to the user's actions. Well, it does. Outside of a text-menu driven console app, there shouldn't be any logic needed to decide what to create. The user chose to click on a button or use a drop down to pick the job type. The UI now has enough information to call a job-specific method and expect a job-specific result. Hence, separate job service classes.



No conditional logic. The logic for SQL 2014 jobs is contained in a class that only has to do with SQL 2014 jobs. Any change to File jobs does not involve anything related to any other job or the services. As written, the services don't really add any value to consumers. They could just use the job constructors themselves. Imagine they had load and save methods, though. Those methods will be specific to the job types.

The logic for constructing the SQL 2014 job's instance name and for rejecting non-root path file job is now in those classes. There is no chance that a change to file job logic could impact other job types.

A Hybrid Approach


Let's say you have a system that looks like the first example that has a single method and for non-technical reasons you cannot easily change the interface. Are you stuck? No.

Change the service to be or use a factory, as in the factory pattern. Job-specific logic is still in separate classes and the service now has just 1 responsibility - to invoke the right service and return the result.

It is a lot more code than the original enum-only version, but it more closely follows the SOLID principles that make software more maintainable. The other classes are the same as in the class version, except the AbstractJob class. It now has an abstract ToJob method that each specific job class must implement to convert itself into a generic Job object. Again, this logic is encapsulated inside each specific class.

Summary


Enums and switches make for less code, but over time it becomes harder to understand and more easily broken. In an object-oriented language classes are there to segregate logic and responsibility for better maintenance. It's a little more code, but you will introduce less bugs.