0

I have a Calc class that I think was improperly placed in the codebase I am working with. For the class structure see code below.

Currently

  • Spec class acts as a storage of data, similar to C struct
  • Calc class is a library of computational functions class that is instantiated as part of other classes, whenever those classes need to do some computation. Calc class also houses Spec class, so that it can do computations using variables of Spec.
  • Plot class is an example of a business object representing a graph.
  • Controller class is a business object representing a controller of sorts in my application, where some computations are made and are displayed in text for user page "View", and a graph is also created which is also displayed to the user on the "View" page.
class Spec
{
    public $a;
    public $b;
    public $c;
}

class Calc
{
    public $spec;

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

    function calcParameter()
    {
        $this->spec->c = $this->spec->a + $this->spec->b;
    }
}

class Plot
{
    public $calc;

    function __construct()
    {
        $this->calc = new Calc();
    }

    function calcPlot()
    {
        $this->calc->spec->c = $this->calc->spec->a * $this->calc->spec->b;
    }
}


class Controller
{
    public $calc;
    public $plot;

    function __construct()
    {
        $this->calc = new Calc();
        $this->plot = new Plot();
    }

    function doBusinessLogic()
    {

        //calc for local
        $this->calc->spec->a = 5;
        $this->calc->spec->b = 5;
        $this->calc->calcParameter();
        print "total is {$this->calc->spec->c}<br>\n";

        //later this format is used by JS to display computational results
        $plotJSON = json_encode($this->calc); 


        //calc for plot
        $this->plot->calc->spec->a = 7;
        $this->plot->calc->spec->b = 143;
        $this->plot->calcPlot();
        print "total is {$this->plot->calc->spec->c}<br>\n";

        //later this format is used by JS to display a plot
        $plotJSON = json_encode($this->plot); 
        print "
        <div id='plot' style='display:none'>$plotJSON</div>
        <script>
            var plot = JSON.parse(document.getElementById('plot').innerHTML);
            document.write('JS says - there are ' + plot.calc.spec.c + ' plot points<br>');
        </script>
        ";
    }  
}

//runs the above
(new Controller())->doBusinessLogic();

JS is used like so:

var plot = JSON.parse(document.getElementById('plot').innerHTML);
document.write('JS says - there are ' + plot.calc.spec.c + ' plot points<br>');

Question

I am under impression that the Calc class was not correctly placed (not designed properly), and as such, it is injected into various places just to do computations. I am thinking that Calc should be a class with no parameters and that Spec should not be part of Calc. And that Calc must only contain functions that do computations, and callers of Calc will supply their own data and receive results. Thereby Calc will become a static library of functions to be used by others. But then is this the best way?

How can I refactor this to minimize coupling? Keeping in mind that JSON format is either to be preserved, or being mindful that any changes in code may require changes to JS code as well.

Before I go knee-deep refactoring this, the design I have now works. Is there a need to refactor at all? Just checking.

Dennis
  • 7,907
  • 11
  • 65
  • 115

1 Answers1

0

Okay, based on what I see so far, the Calc class should not exist at all. The calcParameter method should be a member of the Spec class.

The calcPlot method of Plot should also be a member of the Spec class (it deals entirely with Spec fields as currently written.) You still might want to keep Plot around though if it does other interesting things to its Spec object.

Like this:

class Spec {
    public $a;
    public $b;
    public $c;

    function calcParameter() {
        $this->c = $this->a + $this->b;
    }

    function calcPlot() {
        $this->c = $this->a * $this->b;
    }
}

class Plot {
    public $spec;

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

    function calcPlot() {
        $this->spec->calcPlot()
    }
}


class Controller {
    public $spec;
    public $plot;

    function __construct() {
        $this->spec = new Spec();
        $this->plot = new Plot();
    }

    function doBusinessLogic() {
        //spec for local
        $this->spec->a = 5;
        $this->spec->b = 5;
        $this->spec->calcParameter();
        print "total is {$this->spec->c}<br>\n";

        //later this format is used by JS to display computational results
        $plotJSON = json_encode($this->spec); 

        //spec for plot
        $this->plot->spec->a = 7;
        $this->plot->spec->b = 143;
        $this->plot->calcPlot();
        print "total is {$this->plot->spec->c}<br>\n";

        //later this format is used by JS to display a plot
        $plotJSON = json_encode($this->plot); 
        print "
        <div id='plot' style='display:none'>$plotJSON</div>
        <script>
            var plot = JSON.parse(document.getElementById('plot').innerHTML);
            document.write('JS says - there are ' + plot.spec.c + ' plot points<br>');
        </script>
        ";
    }  
}

//runs the above
(new Controller())->doBusinessLogic();
Daniel T.
  • 32,821
  • 6
  • 50
  • 72
  • As a general rule... If you have a class that only has one field, and the class is not restricting the possible values of its field, then the class shouldn't exist. – Daniel T. Jul 17 '15 at 15:12
  • thanks. what do you mean by "restricting values of its field"? – Dennis Jul 17 '15 at 15:12
  • So, in general, I see this as *move functions away from `Calc` and into their respective classes*, eventually getting rid of `Calc`. I think that `Spec` can definitely use some functions that operate on its data, and also be renamed to better represent an object (as it becomes clearer in refactoring efforts). For example in my full codebase `Spec` can represent products or motors and can be different for each specific product line. I am not 100% sure that `Plot`-anything needs to be in `Spec`. `Plot` is an entity that stores and does operations on graph data and can probably be decoupled. – Dennis Jul 17 '15 at 15:18
  • For example if the class ensures its field is always greater than 0, or some other invariant that the field's class doesn't ensure. – Daniel T. Jul 17 '15 at 15:18