Compute and Display Engineering Information

Question

I am updating a class in legacy code and having some issue with my calcPressure method. I want to make it nice and clean but there is some convolution in the process.

Namely the calcPressure method I have does this:

• computes a value from existing values
• writes that value into an in-class parameter
• returns the value to the caller
• prints debug output while computing
• prepares output for View (JSON, document...innerHtml)

Issues

In general, the code is all interconnected and it works, so part of me feels "why do I want to mess with it". But part of me feels that Separation of Concerns (SOC) is super-broken. After all, the same internal pressure parameter that originates inside PHP class goes all the way into JS view, from being computed in PHP, transferred to JS and displayed in the browser. So if I make pressure private for example, or rename it, my view will become broken, since it depends on having the same parameter name in JS as in PHP for it to work.

More specifically my method seems to do an awful lot. I'm questioning whether I should only have it compute data and return it without assigning the in-class parameter. Or if I should only assign the in-class parameter and not return anything. Right now it does both. I wonder if I should redirect debugging information to be a part of view and not be a part of the method somehow (computation/debug output separation).

Code

class Spec
{

    function load()
    {
         $result = db_query("SELECT * from spec where id = {$this->id}");
         $row = db_fetch_array($result);
         $this->n = $row['n'];
         $this->sg = $row['sg'];
         $this->q = $row['q'];
    }

    /**
     * Computes and returns Pressure
     * Outputs debug info
     * Assigns internal parameter
     * 
     * @return number
     */
    function calcPressure()
    {
        $res = abs($this->n * sqrt($this->q) / 15164.93 * $this->sg);
        dump("  *** <u>Pressure</u> = $res");
        $this->pressure = $res;
        return $res;
    }

    public $pressure;

    public $sg;

    public $q;

    public $n;

    public $id;
}

PHP side

$spec = new Spec();

/*
 * Load of static parameters ($q, $sg, $n) from DB omitted for clarity
 */
$spec->id = 5;
$spec->load();
$spec->calcPressure();
$json = json_encode($spec);

JS/View side

<script>
var x = <?=$json?>;

/*
 * Populates HTML pages with computed pressure information
 */
document.getElementById('pressure').innerHTML = x.pressure;
</script>

Dump

function dump($sql){print "<pre>";print_r($sql);print"</pre>" . PHP_EOL;}

Answer 1

This is a pretty incomplete code example, but let me give you some initial thoughts:

• Use meaningful class and variable names. Spec seems very indeterminate as to what the class does. $sg, $q, and $n also convey little value (though perhaps that is because these are common scientific formula names that I am not fully familiar with.
• Don't have the class output directly to standard output (which is what I assume the dump() function does. Consider actually logging to server logs, or have it returned as part of the method return value so that caller can determine how to display it.
• I almost question the need for this to be a class at all if this is only going to have a single method. It is very unclear how class properties are set. If all you are going to do is instantiate an object, use several methods to set properties (or directly set since these are public), and then call a single method, I don't see why this could not just be a function, or just static class method, as I do not understand what value you get by instantiating the class. Perhaps if you had a constructor where you passed in and validated the variable values or, better yet, interacted with the DB to set these values, there would be value in having a class.
• Do you really want to directly json_encode the object? If you do, you might consider implementing the JSONSerializable interface, which would allow you to implement a jsonSerialize() method where you could specifically control the data structure that is sent to json_encode() for serialization. For example, if you wanted to make properties protected/private but still be able to see them in JSON output, you could build a stdClass object or associative array with key-value pairs describing the object, which would then be serialized.
• If your code needs to be compatible with potentially older versions of PHP (i.e. < 5.4), you should probably not use short echo tag syntax (i.e. <?=$json?>)

Since you know have complete class information, I have updated my answer to give more specific review comments on your code below. My comments are within multi-line comments.

/*
Class name seems non-specific
*/
class Spec
{
/*
Typical style guides would have class properties defined before class methods.
All methods should have visibility (i.e. public, protected, private) explicitly
defined.
Consider using a constructor such that you could do something like:
$spec = new Spec($id);
Where the constructor could call the load method. This would allow you to make
load method private, meaning calling code couldn't destructively reload
the object.  This will also allow you to validate the id input as integer
value greater than zero before trying to do any further work in instantiating
the class. 
If you are going to use PHPDoc, use it on every method and property as well as
class itself. Don't partially use documentation.
*/
    function load()
    {
/*
Consider working with DB in an object-oriented fashion, with database object
dependency being passed to this class on instantiation.
Consider using parametrized prepared statements here rather than string
interpolation.  String interpolation raises question as to whether you might
be opening up an SQL injection vulnerability. 
Don't use 'SELECT *'. It makes expected return results unclear to the code
reader and also potentially negatively impacts performance if you end up
retrieving more fields than necessary.
This DB interaction only considers happy path. What if query fails for some
reason?  What if an empty result set is returned?
*/
         $result = db_query("SELECT * from spec where id = {$this->id}");
         $row = db_fetch_array($result);
         $this->n = $row['n'];
         $this->sg = $row['sg'];
         $this->q = $row['q'];
    }

    /**
     * Computes and returns Pressure
     * Outputs debug info
     * Assigns internal parameter
     * 
/*
"number" is not a valid @return type. It looks like you should be using "float".
*/
     * @return number
     */

/*
I really don't understand why you would need to make this method publicly
available. If you had a constructor, you could pass that constructor the id
(as well as a valid DB object if you choose to work with DB in OO fashion)
and then execute private load and calcPressure methods, setting up the object
for its intended use directly upon instantiation.
*/
    function calcPressure()
    {
        $res = abs($this->n * sqrt($this->q) / 15164.93 * $this->sg);
/*
Don't output from this class.
*/
        dump("  *** <u>Pressure</u> = $res");
        $this->pressure = $res;
/*
You would probably not need a return if you made this private method called
from constructor.
*/
        return $res;
    }
/*
Consider whether these should be public.  My guess is that they shouldn't be
unless you REALLY want the object to be mutable since you are reading this
information from a database and haven't shown an update method.
If you do intend to make the object mutable, then it still probably makes sense to make these private so that you can have proper setters that can trigger
methods to update the database and recalculate pressure.
If you make them private, you would obviously need to implement getters or
__get() magic method to allow caller to get to the information.
As you begin to work more with OOP, you will likely find that the use cases
for truly public properties are fairly limited.
*/
    public $pressure;

    public $sg;

    public $q;

    public $n;

    public $id;
}

Answer 2

First of, you are vulnerable to SQL injection. "remember to check if the value is an integer before setting it" would not be a reasonable defense against injection, you really need to defend in the same place as you build the query, and you need to use prepared statements.

Structure

On the PHP side, the structure seems fine to me. Spec is a classic model, holding the spec data and containing the business logic. The calcPressure method also doesn't do quite as much as you say (it doesn't prepare the output, which would indeed not be good).

But the fields should be private and accessible via getters, which would break your view.

The question is if you actually really need the whole object in the js view. In the example you posted, this is not the case, you could simply do this:

document.getElementById('pressure').innerHTML = <?= $spec->getPressure() ?>;

It's not only cleaner regarding strucure, but it also avoids unnecessary overhead and exposing data you don't actually want to expose (eg the id, possibly).

Of course if for some reason you do need the whole object, @Mike Brants solution using JSONSerializable would be recommended (here you can also define what data you pass on, and using it wouldn't mean having to rewrite your entire application, as the view doesn't need to change).

I'm questioning whether I should only have it compute data and return it without assigning the in-class parameter. Or if I should only assign the in-class parameter and not return anything. Right now it does both

If you always need the pressure, you might as well calculate it when loading the object (as the object wouldn't actually be complete without it). Otherwise, I would just set the value, returning it is unnecessary and may be confusing.

I wonder if I should redirect debugging information to be a part of view and not be a part of the method somehow (computation/debug output separation).

I'm not even sure why you have the debug statement in there, it shouldn't be needed, and it will add a lot of unnecessary overhead in production.

If you want to know if calcPressure works correctly, it's not going to help, you would need automated tests for that. And if you just want to see if something is calculated, you would see that in the view anyways.