Skip to main content
added 3854 characters in body
Source Link
Mike Brant
  • 9.9k
  • 14
  • 24

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;
}

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;
}
Source Link
Mike Brant
  • 9.9k
  • 14
  • 24

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?>)