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”…..
3
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.
5