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".....