Reducing the complexity of a class

Question

I have looked at some answers and searched on Google, but I could not find anything helpful (i.e., that wouldn't have awkward side effects).

My problem, in abstract, is that I have an object and need to perform a long sequence of operations on it; I think of it as a sort of assembly line, like building a car.

I believe these objects would be called Method Objects.

So in this example at some point I would have a CarWithoutUpholstery on which I would then need to run installBackSeat, installFrontSeat, installWoodenInserts (the operations do not interfere with one another, and might even be done in parallel). These operations are performed by CarWithoutUpholstery.worker() and yield a new object which would be a CarWithUpholstery, on which I would then run maybe cleanInsides(), verifyNoUpholsteryDefects(), and so on.

The operations in a single phase are already independent, i.e., I am already grappling with a subset of them that may be executed in any order (front and rear seats may be installed in any order).

My logic currently uses Reflection for simplicity of implementation.

That is, once I have a CarWithoutUpholstery, the object inspects itself for methods called performSomething(). At that point it executes all these methods:

myObject.perform001SomeOperation();
myObject.perform002SomeOtherOperation();
...

while checking errors and stuff. While order of operation is unimportant, I have assigned a lexicographic order in case I ever discover some order is important after all. ^{_{This contradicts YAGNI, but it costed very little - a simple sort() - and it could save a massive method renaming (or introducing some other method of performing tests, e.g. an array of methods) down the line.}}

A different example

Let us say that instead of building a car I have to compile a Secret Police report on somebody, and submit it to my Evil Overlord. My final object will be a ReadyReport. To construct it I start by gathering basic information (name, surname, spouse...). This is my Phase A. Depending on whether there is a spouse or not, I may then have to proceed to phases B1 or B2, and gather sexuality data on one or two people. This is made of several different queries to different Evil Minions controlling night life, street cams, sex shop sales receipts and what not. And so on and so forth.

If the victim has no family, I will not even enter the GetInformationAboutFamily phase, but if I do, then it's irrelevant whether I first target the father or the mother or the siblings (if any). But I cannot do that if I haven't performed a FamilyStatusCheck, which therefore belongs to an earlier phase.

It all works wonderfully...

• if I need some additional operation I only need to add a private method,
• if the operation is common to several phases I can have it inherited from a superclass,
• operations are simple and self-contained. No value from one operation is ever required by any of the others (operations that do are performed in a different phase),
• objects down the line do not need to perform many tests since they could not even exist if their creator objects hadn't verified those conditions in the first place. I.e., when placing inserts in the dashboard, cleaning the dashboard and verifying the dashboard, I needn't verify that a dashboard is actually there.
• it allows for easy testing. I can easily mock a partial object and run any method on it, and all operations are deterministic black boxes.

...but...

The problem arose when I added one last operation in one of my method objects, which caused the overall module to exceed a mandatory complexity index ("less than N private methods").

I have already taken the matter upstairs and suggested that in this case, the wealth of private methods is not a telltale of disaster. The complexity is there, but it's there because the operation is complex, and actually it's not all that complex - it's just long.

Using the Evil Overlord example, my problem is that the Evil Overlord (aka He Who Shall Not Be Denied) having requested all dietary information, my Dietary Minions telling me that I need to query restaurants, kitchenettes, street vendors, unlicensed street vendors, greenhouse owners etc., and the Evil (sub)Overlord - familiarly known as He Who Also Shall Not Be Denied - complaining that I'm performing too many queries in the GetDietaryInformation phase.

Note: I am aware that from several points of view this is not a problem at all (ignoring possible performance issues etc.). All that's happening is that a specific metric is unhappy, and there is justification for that.

What I think I could do

Apart from the first, all these options are doable and, I think, defensible.

• I have verified that I can be sneaky and declare half my methods protected. But I'd be exploiting a weakness in the testing procedure, and apart from justifying myself when caught, I don't like this. Also, it's a stopgap measure. What if the number of required operations doubles? Unlikely, but what then?
• I can split arbitrarily this phase into AnnealedObjectAlpha, AnnealedObjectBravo and AnnealedObjectCharlie, and have one third the operations being performed at each stage. I am under the impression that this actually adds complexity (N-1 more classes), with no benefit except for passing a test. I can of course hold that a CarWithFrontSeatsInstalled and a CarWithAllSeatsInstalled are logically successive stages. The risk of a Bravo method being later required by Alpha is small, and even smaller if I play it well. But still.
• I can bunch different operations, remotely similar, in a single one. performAllSeatsInstallation(). This is only a stopgap measure and it does increase the complexity of the single operation. If I ever need to do operations A and B in a different order, and I have packed them inside E = (A+C) and F (B+D), I'll have to unbundle E and F and shuffle the code around.
• I can use an array of lambda functions and neatly sidestep the check altogether, but I find that clunky. This is however the best alternative so far. It would get rid of reflection. The two problems I have is that I would probably be asked to rewrite all method objects, not only the hypothetic CarWithEngineInstalled, and while that would be very good job security, it really doesn't appeal all that much; and that the code coverage checker has issues with lambdas (which are solvable, but still).

So...

• Which, you think, is my best option?
• Is there a better way I haven't considered? (^{_{perhaps I'd better come clean and ask directly what is it?}})
• Is this design hopelessly flawed, and I'd better admit defeat and ditch - this architecture altogether? ^{_{Not good for my career, but would writing ill-designed code be any better in the long term?}}
• Is my current choice actually the One True Way, and I need to fight to get better quality metrics (and/or instrumentation) installed? For this last option I'd need references... I can't just wave my hand at the @PHB while murmuring These aren't the metrics you're looking for. ^{_{No matter how much I'd like to be able to}}

Answer 1

The long sequence of operations seems like it should be its own class, which then needs additional objects to perform its duties. It sounds like you are close to this solution. This long procedure can be broken down into multiple steps or phases. Each step or phase could be its own class that exposes a public method. You would want each phase to implement the same interface. Then your assembly line keeps track of a list of phases.

To build on your car assembly example, imagine the Car class:

public class Car
{
    public IChassis Chassis { get; private set; }
    public Dashboard { get; private set; }
    public IList<Seat> Seats { get; private set; }

    public Car()
    {
        Seats = new List<Seat>();
    }

    public void AddChassis(IChassis chassis)
    {
        Chassis = chassis;
    }

    public void AddDashboard(Dashboard dashboard)
    {
        Dashboard = dashboard;
    }
}

I'm going to leave out the implementations of some of the components, like IChassis, Seat and Dashboard for the sake of brevity.

The Car is not responsible for building itself. The assembly line is, so let's encapsulate that in a class:

public class CarAssembler
{
    protected List<IAssemblyPhase> Phases { get; private set; }

    public CarAssembler()
    {
        Phases = new List<IAssemblyPhase>()
        {
            new ChassisAssemblyPhase(),
            new DashboardAssemblyPhase(),
            new SeatAssemblyPhase()
        };
    }

    public void Assemble(Car car)
    {
        foreach (IAssemblyPhase phase in Phases)
        {
            phase.Assemble(car);
        }
    }
}

The important distinction here is that the CarAssembler has a list of assembly phase objects that implement IAssemblyPhase. You now are dealing explicitly with public methods, meaning each phase can be tested by itself, and your code quality tool is happier because you aren't stuffing so much into one single class. The assembly process is dead simple too. Just loop over the phases and call Assemble passing in the car. Done. The IAssemblyPhase interface is also dead simple with just a single public method that takes a Car object:

public interface IAssemblyPhase
{
    void Assemble(Car car);
}

Now we need our concrete classes implementing this interface for each specific phase:

public class ChassisAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        car.AddChassis(new UnibodyChassis());
    }
}

public class DashboardAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        Dashboard dashboard = new Dashboard();

        dashboard.AddComponent(new Speedometer());
        dashboard.AddComponent(new FuelGuage());
        dashboard.Trim = DashboardTrimType.Aluminum;

        car.AddDashboard(dashboard);
    }
}

public class SeatAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
    }
}

Each phase individually can be pretty simple. Some are just one liners. Some might be just blinding adding four seats, and another has to configure the dashboard before adding it to the car. The complexity of this long drawn out process is split into multiple, reusable classes that are easily testable.

Even though you said the order doesn't matter right now, it might in the future.

To finish this off, let's look at the process for assembling a car:

Car car = new Car();
CarAssembler assemblyLine = new CarAssembler();

assemblyLine.Assemble(car);

You can sub class CarAssembler to assemble a specific kind of car, or truck. Each phase is one unit within a larger whole, making it easier to reuse code.

---

Is this design hopelessly flawed, and I'd better admit defeat and ditch - this architecture altogether? Not good for my career, but would writing ill-designed code be any better in the long term?

If deciding what I wrote needs to be rewritten was a black mark on my career, then I would be working as a ditch digger right now, and you shouldn't take any of my advice. :)

Software engineering is an eternal process of learning, applying, and then re-learning. Just because you decide to rewrite your code doesn't mean you're going to get fired. If that were the case, there would be no software engineers.

Is my current choice actually the One True Way, and I need to fight to get better quality metrics (and/or instrumentation) installed?

What you have outlined is not the One True Way, however bear in mind that code metrics are also not the One True Way. Code metrics should be viewed as warnings. They can point to maintenance problems in the future. They are guidelines, not laws. When your code metrics tool points something out, I would first investigate the code to see if it properly implements the SOLID principals. Many times refactoring code to make it more SOLID will satisfy the code metrics tools. Sometimes it doesn't. You have to take these metrics on a case by case basis.

Answer 2

I don't know if this is possible for you, but I'd be thinking about using a more data oriented approach. The idea is that you capture some (declarative) expression of rules & constraints, operations, dependencies, state, etc... Then your classes and code are more generic, oriented around following the rules, initializing current state of instances, performing operations to change state, by using the declarative capture of rules and state changes rather than using code more directly. One might use data declarations in the programming language, or, some DSL tooling, or a rules or logic engine.

Answer 3

Somehow the problem reminds me of dependencies. You want a car in a specific configuration. Like Greg Burghardt, I'd go with objects/classes for each step/item/….

Each step declares what it adds to the mix (what it provides)(can be multiple things), but also what it requires.

Then, you define the final required configuration somewhere, and have an algorithm that looks at what you need, and decides which steps need to executed/what parts need to be added/which documents need to be gathered.

Dependency resolution algorithms are not that hard. The simplest case is where each step provides one thing, and no two things provide the same thing, and the dependencies are simple (no 'or' in the dependencies). For the more complex cases: just look at a tool like debian apt or something.

Finally, building your car then reduces to what possible steps there are, and letting the CarBuilder figure out the steps needed.

One important thing however is that you must find a way to let the CarBuilder know about all the steps. Try to do this using a configuration file. Adding an extra step would then only need an addition to the config file. If you need logic, try adding a DSL in the config file. If all else fails, you're stuck defining them in the code.

Answer 4

A couple of things you mention stand out as 'bad' to me

"My logic currently uses Reflection for simplicity of implementation"

There really is no excuse for this. are you really using string comparison of method names to determine what to run?! if you change the order will you have to change the name of the method?!

"The operations in a single phase are already independent"

If they are truly independent of each other it suggests that your class has taken on more than one responsibility. To me, both your examples seem to lend themselves to some sort of single

MyObject.AddComponent(IComponent component) 

approach which would allow you to split the logic for each opperation into its own class or classes.

In fact I imagine they are not truly independent, I imagine that each examines the state of the object and modifies it, or at least performs some sort of validation before commencing.

In this case you can either:

1. Throw OOP out the window, and have services which operate on the exposed data in your class ie.

   Service.AddDoorToCar(Car car)

2. Have a builder class which constructs your object by assembling the needed data first and passing back the completed object ie.

   Car = CarBuilder.WithDoor().WithDoor().WithWheels();

(the withX logic can again be split into subbuilders)

3. Take Functional approach and pass functions/delgates into a modify method

   Car.Modify((c)=> c.doors++);