Object Design & Cohesion - Issue & potential refactoring

Question

Abstract

I have been trying to get my head around the cohesiveness of some functionality in our code base. I’ve approached this design in different ways, and lately I’m convinced that I took the wrong approach, as in I incorrectly applied the Single Responsibility Principle.

Issue

The problem domain is a Well, otherwise known as a round hole in the ground. The current code looks something like this... I have a set of data in a data only object...

Note: This is not the full code, just method names to keep things short...

public class WellData
{
    public string Name { get; set; }
    public ICollection<SurveyPoint> SurveyPoints { get; set; }
    public ICollection<GeometryItem> GeometryItems { get; set; }
    public ICollection<TemperaturePoint> WellTemperature { get; set; }
    public ICollection<FluidPoint> WellFluids { get; set; }
}

I broke functionality that operates on this data into several small classes. As per below…

public class ReferenceWellSurvey
{
   ICollection<SurveyPointCalculate> _calculatedSurveyPoints;
   double GetTvdAtDepth(double depth) {}
   double GetAzimuthAtDepthRad(double depth) {}
   double GetInclinationAtDepthRad(double depth){}
   double GetTortuosityPeriodAtDepth(double  depth ) {}
   double GetTortuosityAmplitudeAtDepth(double depth ) {}
}

public class ReferenceWellGeometry
{
   ICollection<GeometryItem> _geometryItems;
   double GetFrictionAtDepth(double depth) {}
   double GetHoleIdAtDepth(double depth) {}
}

public class ReferenceWellTemperature
{
   ICollection<TemperaturePoint> _wellTemperatures;
   double GetTemperatureAtDepth(double depth) {}
   double GetSurfaceTemperature(double depth) {}
}
Etc.

I’ve realized lately when looking at our code that I have to glue all this back together to make it usable where I need it in the application logic. Something like….

public class ReferenceWellData
{
private IReferenceWellGeometry _refWellGeometry;
private IReferenceWellTemperature _refWellGeometry;
          private IReferenceWell _refWellGeometry;
          private IReferenceFluid _refWellFluid;
       //Proxy functions from each class to glue it all back together…
}

A few things that led me to the current mess. The well data class contains several collections of information and if everything is in one class most functions will only operate on a small subset of the data. At the time I felt that would create low cohesion, now a few months later, I feel these things do actually all belong together. Additionally, there are several classes in my code that need to call just one method of the ReferenceWellSurvey class, GetTvdAtDepth(). Really what I did didn’t solve this issue.

After thinking more about this, and after reading this Wikipedia article http://en.wikipedia.org/wiki/Cohesion_(computer_science), I’m starting to see that I have committed a few programming sins.

• Anemic domain model

• Low cohesion

• Constructor over injection in code where I try to glue things together

• Giving a class that depends on GetTvdAtDepth access to other methods that it does not need, violated Interface Segregation Principle.

Solution

I’m looking for that Goldilox design that doesn’t give too much responsibility to any one class (No God Objects), while yet maintaining a good degree of cohesion.
My current thoughts on fixing this are as follows. Move the functionality that operates on the well data into the same class. This should help get this functionality closer to Communicational/informational cohesion.

public class WellData : IWellData 
{
    public string Name { get; set; }
    public ICollection<SurveyPoint> SurveyPoints { get; set; }
    public ICollection<GeometryItem> GeometryItems { get; set; }
    public ICollection<TemperaturePoint> WellTemperature { get; set; }
    public ICollection<FluidPoint> WellFluids { get; set; }

   // Operates on SurveyPoint collection 
   double GetTvdAtDepth(double depth) {}
   double GetAzimuthAtDepthRad(double depth) {}
   double GetInclinationAtDepthRad(double depth){}
   double GetTortuosityPeriodAtDepth(double  depth ) {}
   double GetTortuosityAmplitudeAtDepth(double depth ) {}
   //Operates on WellGeometry collection 
   double GetFrictionAtDepth(double depth) {}
   double GetHoleIdAtDepth(double depth) {}
   //Operates on WellTemperature collection
   double GetTemperatureAtDepth(double depth) {}
   double GetSurfaceTemperature(double depth) {}
}

Next to address the issue of several classes needing just one function, I could make this one function into a separate interface IGetTvdAtDepth. In this way, my classes that need that functionality can depend just on this interface, which I can inherit into my WellData interface, and implement it there, but not tie my other classes to all the functionality that is in my WellData class.

Thoughts on this approach vs. the original approach? One thing that bothers me about doing things way is that in my API I'm using bastard injection to fulfill the required dependencies of this class... Right now I have three required dependencies, a Survey calculator, and a special class that references data at different measured depths. I'd like to move away from this, but that seems to point back toward having services do more of the lifting...

Answer 1

Start with deeply defining your classes instead of a Uber-Interface obstruction abstraction.

I think of it as defining my "oil well domain" data structures.

The idea is to discover the bits that have independent/discrete meaning. As the class definitions evolve, extract parts that can in their own right be classes. Then composite as needed later. You always edit classes as things develop. This approach should allow us to evolve higher-level classes that address cross-cutting concerns.

• Is a ReferenceWell a Well or a derived Well? Static or "reference" data/state makes it just another Well.
• HoleID uniquely identifies a Well object

Could this be what a SurveyPoint is?

public class SurveyPoint
{
    // Each property is typed. This is intentional.
    // It gives us a place to encapsulate properties and
    // behaviors/methods that are "atomic" to each.

    public int              HoleID      { get; set; }
    public Geometry         Geometry    {get; set;}
    public Depth            Depth       {get; set;}
    public TemperaturePoint Temperature { get; set; }
    public Inclination      Inclination { get; set; }
    public Tortousity       Torture     { get; set; }
    public Asmuth           Asmuth      { get; set; }
    public Tvd              Tvd         { get; set; }
}

And then TemperaturePoint, FluidPoint, GeometryItem, etc. are just subsets (not sub classes) of a SurveyPoint? In other words, different aspects. So..

public class SurveyPoint
{
    //. . .

    public Temperature GetTemperature() {return this.Temperature;}
    public FluidPoint GetFluidPoint () {return this.FluidPoint;}
    // etc. and overload as needed.
}

public class SurveyPointCollection : Collection<SurveyPoint> {
    public TemperatureCollection GetTemperatures() {}
    public FluidPointCollection GetFluidPoints() {}

    public GetTemperature(Depth depth) {
       // iterate collection to find the SurveyPoint with that depth
     }
    //etc.
}

public class Temperature {
    public HoleID {get; set;}
    public double Temperature {get; set;}
    // any other properties help *define* a temperature for a well

    // add any useful methods that operate only on properties defined in this class
}

// a similar class for each of the other SurveyPoint aspects.

I see one aspect of cohesion evolving and revolving around the core data structures. Once the data classes are described in (enough) detail then you can begin to "map" behavior to appropriate levels of the data encapsulations thus making very cohesive Object Oriented classes (yes @FrankHileman is correct, you are thinking functionally not Objectively).

Then I assume a Well might start looking like this

public class Well
{
    public string Name {get; set;}
    public int HoleID {get; set;}
    public SurveyPointCollection SurveyPoints {get; set;}

    public Well () { }

    public TemperatureCollection GetTemperatures () { 
         SurveyPoints.GetTemperatures();
    }

    public Temperature GetTemperature ( Depth depth ) { }

    // etc. for all other aspects.

}

Exploring the Behavior Aspects

public class Surveyer
{
    protected Well MyWell { get; set; }
    protected DepthCalculator Calculator { get; set; }

    public Surveyer ( Well theWell ) {
        MyWell = theWell;
        Calculator = new DepthCalculator (MyWell.GetSurveyPoints());
    } 
}

"Depth" seems to be the unifying thing in the ReferenceWellSurvey method names so I imagined a DepthCalculator. However we may very well have calculation that corresponds to our various data aspects. So we might have this:

public class DepthCalculator {
    protected TemperatureCalculator {get; set;}
    protected InclinationCalculator {get; set;}
    protected FrictionCalculator    {get; set;}
    // etc.

    public DepthCalculator (SurveyPointCollection dataPoints) {
        TemperatureCalculator = new TemperatureCalculator (dataPoints.GetTemperatures());
        FrictionCalculator = new FrictionCalculator(dataPoints.GetFrictionPoints());
    }
}

I don't know enough about the "oil well domain" to know where to go with this. However you might look at the Builder Pattern for inspiration.

Answer 2

Sometimes design patterns and design guidelines can get in the way of simplicity. The problem you describe (the original problem solved by the software, not the design concerns) sounds like it is best solved by a custom language consisting of composable functions. I use the word "language" in the broader sense, not to imply you need a parser, etc. Your objects hold data, and functions provide the operations on that data. This is not an object-oriented analysis, but a functional analysis. If you then wish to be more object-oriented, you can try to assign some functions to some objects that also encapsulate data. Probably the functional design should come first, then the object-oriented design.

Answer 3

I think you committed much worse sin in your work : you didn't write any test. From much more abstract way, you didn't look at your code from the perspective of code which will use it. It is much easier to see abstractions and possible ways to divide the behavior if you actually write the code that uses what you are trying to design. So it leads to actually better design.

Next thing to note is that while SRP is nice, it should not be used without the rest of the OLID principles. SRP reminds us that there is something called responsibility and that we should be aware of it. But it is extremely bad rule to base your design on, simply because responsibility can mean anything from "responsibility to add two numbers" to "responsibility to solve all customer's problems". This makes SRP quite ambiguous. Software design is complex discipline and basing it on just one principle is foolish at best.

Answer 4

Do the functional classes have any real internal state or possible variations / alternative implementations? If not, could you rewrite ReferenceWellGeometry to be a utility class with all static methods that take one additional parameter for the collection? e.g.

static double GetFrictionAtDepth( ICollection<GeometryItem> _geometryItems, double depth) {}

This style has drawbacks, but for your case it might simplify all the class dependency / injection issues.

Answer 5

Note

Any answer will be biased by the description of the problem/model we were given; there may be other ways of looking at the domain that are more helpful

Caveats aside

It looks like you have a Well class and several collections of related Measurements on a well. If the measurements may be repeated at different times, then any single Well may have several sets of Measurements, even of the same kind

this leads to a simple structure: an object and a set of measurements/calculations on the object -

Well - a concrete class for an instance of a well, including properties intrinsic to the well (like HoleID or Name)
WellOperation - an abstract class for a set of related Measurements on a specific Well
    ReferenceWellTemperature - a bunch of temperature measurements
    ReferenceWellGeometry - a bunch of geometry measurements
    etc.
    

Each operational subclass contains data in reference to a particular well. A single well may have zero or more of these, possibly multiples of the same kind taken at different points in time

So you do not need - and do not want - an overarching class like the ReferenceWellData example; the number and kinds of measurement operations may change, which will cause the overarching class' interface to change, and that's not necessary

Answer 6

If you didn't need ReferenceWellSurvey, ReferenceWellGeometry, etc... to communicate then it made sense (from an implementation point of view) to do this break-down. However it is not advisable to rely on implementation level details to establish your class structure.

"I’ve realized lately when looking at our code that I have to glue all this back together to make it usable where I need it in the application logic." You mean, because the user of this class doesn't care ( and so, would better not be exposed to ) about the implementation level decomposition? But glueing things in this way isn't necessarily bad.

"A few things that led me to the current mess"... What current mess? The fact that a collection of classes, each in charge of a small set of related data, support the implementation of another class, doesn't constitute a mess. Now, if you need to import ReferenceWellSurvey from ReferenceWellGeometry (or any similar dependencies between these supportive classes), well that would be a mess and would undermine your 'implementation oriented' breakdown.

• WellData should live on (is there a reason not to name this 'Well'? Are there concurrent models of a well in your data) whether you keep the service oriented breakdown or not. A well designed class is a class that's intuitively named and exposes useful functionality.
• IWellData may live on. Assuming anything is using it.
• If your breakdown into service oriented classes already started feeling awkward, get rid of it. Try to look at this from the point of a newbie trying to maintain your code. A poor old Well object will be a delight for them.
• IGetTvdAtDepth (rename to ITvdCalculator? Is Tvd an acronym?) looks okay, just make sure that hiding remaining functionality is legitimate (if somebody wanted to access another Well method in the future, would be fine? In other words, are you hiding these methods because they are not needed, or because they should not be accessed?)

"One thing that bothers me about doing things way is that in my API I'm using bastard injection to fulfill the required dependencies of this class... Right now I have three required dependencies" Is this a problem? If so, that might provide the rationale for defining IWellData.

In conclusion I think your proposed solution may be fine. Would this go against the responsibility principle? Maybe, but I think a design that endorses the definition of 'Well' in your domain is a better design than creating implementation level classes.