5

I have a class that acts as a library of functions for various products. To compute its data, the function currently needs to be aware of all product names. Depending on which product calls the function, it returns product-specific answers.

The codebase is convoluted enough to where classes are tied to each other in round-about ways and are messy and have no clear immediately-recognizable structure. So I am refactoring often without a specific purpose in mind, but in this case I want to refactor to

  • reduce the library's need to rely on product knowledge
  • maybe I can clean up the code enough to where I can possibly use polymorphism - instantiate a specific product and then have a generic-looking codeblock call product-specific functions

Whether I need to do this type of refactoring, I don't know, as afterall "Things work fine now", except that they are hard to follow and bugs are hard to track down. Nevertheless, my goal and my question is to "remove reliance on specific product knowledge from calcVar function".

Question: How do I remove product awareness from calcVar() function below.

Working Example Below:

class Product{}

class Graph{}

class ProductA extends Product
{
    public $calc;

    public function __construct() {
        $this->calc = new CalcLibrary();
        $this->calc->spec->product = "A";
    }
}

class GraphB extends Graph
{
    public $calc;

    public function __construct()    {
        $this->calc = new CalcLibrary();
        $this->calc->spec->product = "B";
    }
}

class Specs
{
    public $var;
    public $product;

    //loads different specs based on product name
    function __construct() {
        if ($this->product == "A") $this->var = 3;
        if ($this->product == "B") $this->var = 4;
        $this->var = 5;
    }
}

class CalcLibrary
{
    public $spec;

    public function __construct() {
        $this->spec = new Specs();
    }

    //problem - library holding formulas for "Var" is product-aware
    //loads different formulas based on product name
    public function calcVar() {
        if ($this->spec->product == "A")
        {
            //somewhat different formulas for different products
            if ($this->spec->var <= 5) return 3 * $this->spec->var;
            else return 0;
        }        
        if ($this->spec->product == "B")
        {
            if ($this->spec->var == 0) return 16;
            else return 2 * $this->spec->var;
        }
        return $this->spec->var;
    }

    public function output() {
        echo $this->calcVar() . "\n";
    }
}

// tests should output 15, 10, and 5 
(new ProductA())->calc->output();
(new GraphB())->calc->output();
(new CalcLibrary())->output();

My Thoughts On Solution

Solution Attempt 1 At first solution was easy I thought -- when I was not aware of family of Graph classes

  • move calcVar() from CalcLibary into Product class
  • get rid of if/then/else statements and leave the generic version of calcVar() inside Product class, but move specific ones into ProductA/B/C (classes that extend Product)

and all was well until I found out the hard way that there is a group of classes called GraphA/B/C, which also use CalcLibrary.

Solution 1 Recovery Attempt

Natural solution was to copy CalcVar() into Graph and specialize basically use the same approach as with Product. But, then the Problem is that I will have multiple CalcVar() functions, generic ones in Product, and in Graph, and specialized ones in each of their subclasses. As in a problem of duplication, when right now I don't have duplication of calcVar(). I didn't want to trade one problem for another.

Solution 2 Idea

Another way was to make CalcLibrary the top class, and have Graph and Product classes extend CalcLibrary, have CalcLibrary contain calcVar, as it did before, but now CalcLibrary will only have the generic version of calcVar, and then have individual ProductA/B/C and GraphA/B/C classes have product-specific calcVar() versions. This seems like a good solution as it removes reliance on product knowledge and it takes care of duplication. But I am not 100% sure that extending this library is a good idea. It is not really a library, if I don't want it to be. It's a custom class, so I can make it whatever I need it to be.

Solution 3 Sketch

Maybe all I need is to come up with a better name, so instead of CalcLibrary use ClassThatContainsVariousFunctionsUsefulToGraphAndProductClasses, and then use solution #2. Maybe later through refactoring I will see clearly enough into my codebase to see a better fit of classes (find a better design), but what are your thoughts now, on refactoring this particular example with my needs in mind? Is "Solution 2" what I should go for? Since it's not as easy to just move CalcLibrary to the top, with all of its other functions and various other side-effects, maybe I can create a new class with calcVar function there, and then have Product and Graph extend that new class, leave CalcLibrary alone, and remove unused parts of it as I move things into that new class I've created...

Final Thoughts

Because otherwise, I fail to see a way for Plot classes and Graph classes to use functions inside CalcLibrary, without moving those functions inside the respective product-specific classes somehow. (I mean other than CalcLibrary is being used now, being in a separate class, but with the price of being product-aware)

ALso, ... my 3rd solution sketch with creating Graph/Product extends NewCalcType feels (maybe right) but kind of esoteric.... as in I do not see a meaning other than making existing situation work with an OO design. I mean another idea is to dismantle the class completely and have each class do their own calcVar computations directly inside the classes. There will be lots of duplication, but it will feel more "natural".....

  • How different are each of the products? Are they different enough where you really need separate classes, or can you just use one Product class with a ProductName property? – Robert Harvey Jan 22 '15 at 17:25
  • they are fairly different I would say. for example, each has its own unique properties (several) and some computations differ as well. So if I use one Product class I'd need to have a differentiation on when can I use set of properties for product A, when for product B and so on, while leaving non-active properties unused. – Dennis Jan 22 '15 at 19:19
  • 1
    In response to your comment about not needing to refactor: only refactor when there is a reason that involves making the software better. Refactoring to make bugs easier to find and fix is a valid reason, as is refactoring to reduce coupling such that fixing one bug does not cause two more. Go forth and refactor this code with the blessing of the Internet! – user22815 Jan 22 '15 at 22:57
1

Somewhere in your code you have to store the specifics of each product. In your example, the natural place for this seems to be the Specs class, since I guess it is something like the "configuration" of your library. I would extend that class, for example, by an attribute "calcFactor", so your calcVar function will look like this:

public function calcVar()    {
    return $this->spec->calcFactor * $this->spec->var;        
}

What you need is a factory for creating Spec objects for each product, something like

class SpecFactory
{
    public function CreateSpec(productName) {
        spec = new Specs();
        spec->product=productName;
        if (productName == "A") 
        {
            spec->calcFactor= 3;        
            // ... more parameters for product A here
        }
        else (productName == "B")
        {
            spec->calcFactor= 2;
            // ... more parameters for product B here
        }
        else
        {
           spec->calcFactor= 1; // default
        }
        return spec; 
    }

then your final code will look like this:

class ProductA extends Product
{
    public $calc;

    public function __construct() {
        spec = SpecFactory->CreateSpec("A");  // sets spec->calcFactor to 3 
        $this->calc = new CalcLibrary(spec); 
    }
}

or

class GraphB extends Graph
{
    public $calc;

    public function __construct()    {
        spec = SpecFactory->CreateSpec("A");  // sets spec->calcFactor to 2
        $this->calc = new CalcLibrary(spec); 
    }
}

The code of your factory will be fairly simple, it will contain the one and only "switch" between different products in your whole program. Your library then gets the spec "injected" (simple dependency injection), and whenever there is something product specific, it will consult the spec to provide the parameters. The spec parameters may be more complicated, maybe a polymorphic object with a derivation for each product, to support more complex scenarios. You probably know already how this is called: it is the well known strategy pattern. As a "lightweight" form of the strategy pattern, you could also try if simple "function objects" suits your needs (AFAIK PHP supports this kind of functional stuff).

In fact, how your spec class has to look like might be different in reality. I don't know how it looks like today, maybe it contains a lot of product independent stuff, if that's the case you might separate the product specific configuration and the product independent configuration into separate "specs" classes, but I hope you get the general idea: separate all product specific things into one piece of configuration which is simple and easy to handle, and let the library use that configuration.

  • Surprisingly, PHP does support what they call "anonymous functions" even allowing them to be assigned to variables php.net/manual/en/functions.anonymous.php – Michael Brown Jan 22 '15 at 19:24
  • thanks. I am still processing your answer. To me it also seems that creating product-specific subclasses of CalcLibrary may work. But true, if I can make CalcLibrary product-generic, and push all of the product specifics into Spec class that will work as well. My real Spec class actually already has if/then/else statements loading various specs based on product name. But then, so does CalcLibrary which "loads" different formulas for different products. Because of this I may not be able to simplify calcVar into product generic code. i.e. see the updated example of calcVar. – Dennis Jan 22 '15 at 20:34
  • updated Spec as well to show that both Spec and calcVar have product-specific code inside but for different reasons -- loading product-specific specs to use in formulas, and loading different formulas to use for different products – Dennis Jan 22 '15 at 20:44
  • maybe do a double dependency injection? ... inject Spec into CalcLibrary, then inject CalcLibrary into Product/Graph? – Dennis Jan 22 '15 at 20:49
  • 1
    @Dennis: discussing this by artficial examples does only work to a certain degree, and I think we are beyond that point. This all boils down to how much of the code in your real "Specs" and "CalcVariable" is really product dependent, how much is not, and how to choose the responsibilities between those classes correctly. I won't be able to solve this here for you on programmers, but maybe I could give you a push into the right direction. – Doc Brown Jan 22 '15 at 22:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.