PHP classes to model products, parts, and stock

Question

I am creating some classes, but I don't know if this approach is very solid. First some basic information. I use code igniter as framework. I use $CI =& get_instance() since I need to load some models.

I store products inside a Product class like this:

class Product {
    private $id;
    private $partNumber;
    private $description;
    private $stock;

    /**
     * Product constructor.
     * @param $id
     * @param $partNumber
     * @param $description
     */
    public function __construct($id, $partNumber, $description){
        include_once(APPPATH.'controllers/Core_Stock_Controller_Test.php');

        $this->id = $id;
        $this->partNumber = $partNumber;
        $this->description = $description;
    }

    /**
     * @return mixed
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * @return mixed
     */
    public function getPartNumber()
    {
        return $this->partNumber;
    }

    /**
     * @return mixed
     */
    public function getDescription()
    {
        return $this->description;
    }

    /**
     * @return Stock
     */
    public function getStock(){
        if($this->stock == null){
            $stockController = new Core_Stock_Controller_Test();
            $this->stock = $stockController->getStockFromProduct($this);
        }

        return $this->stock;
    }
}

The controller:

class Core_Product_Beta{

    public function __construct()
    {
        $CI =& get_instance();
        $CI->load->model('Core_Product_Model');
        include_once(APPPATH . 'models/Product.php');
    }

    private function getInstance()
    {
        return get_instance();
    }

    /**
     * @var Int $partId
     * @var Int $partNumber
     * @return bool|Product
     */
    public function getPart($partId = null, $partNumber = null)
    {
        if ($partId) {
            return $this->mapProductToObject($this->getInstance()->Core_Product_Model->getPartnumberFromId($partId));
        }

        if (strlen($partNumber) == 12 && is_numeric($partNumber)) {
            return $this->mapProductToObject($this->getInstance()->Core_Product_Model->getIdFromPartnumber($partNumber));
        }

        return false;
    }

    private function mapProductToObject($data)
    {
        if(isset($data[0])) {
            return new Product($data[0]->id, $data[0]->Part_number, $data[0]->Description, $data[0]->Type, $data[0]->Housing, $data[0]->Gki, $data[0]->enabled);
        } else {
            throw new ProductException("Error with product", "The product does not exist", null);
        }
    }
}

And I load the stock when the method is called, because I don't need it every time (So I think it's faster this way, because sometimes I need more than 200 Products). Is it the right way to create another object in a class?

The Stock controller:

class Core_Stock_Controller_Test{
    public function __construct() {
        $CI =& get_instance();

        $CI->load->model('Core_Stock_Model');
    }

    private function getInstance(){
        return get_instance();
    }

    public function getStockFromProduct(Product $product){
        $result = $this->getInstance()->Core_Stock_Model->getStockForAllLocationPerPartId($product->getId());
        $locations = array();
        $totalStock = 0;

        foreach($result as $res){
            $totalStock = $res["total"];
            $location = new Location($res["location_id"], $res["cd_mag"], $res["cd_location"], $res["cd_loc_type"], $res["stock"]);
            array_push($locations, $location);
        }

        return new Stock($locations, $totalStock);
    }

}

The Stock class:

class Stock
{
    private $locations = array();
    private $totalStock = 0;

    /**
     * Stock constructor.
     * @param array $location
     */
    public function __construct(array $location)
    {
        $this->locations = $location;
    }

    /**
     * @return array
     */
    public function getLocations()
    {
        return $this->locations;
    }

    /**
     * @return Location
     */
    public function getLocation($index){
        return $this->locations[$index];
    }

    /**
     *
     */
    public function getTotalStock(){
        $stock = 0;

        foreach($this->locations as $location){
            $stock += $location->getStock();
        }

        return $stock;
    }
}

This works, but bad because I don't use dependency injection. How can I solve this? I do it this way because I want to delegate everything.

So that I can simply say:

$controller = new Core_Product_Beta();
$product = $controller->getPart(1, null);

No issues till now. I want to get the stock from that product, and I want to simply call $product->getStock, without instantiating and adding classes to each other at this level.

Another reason why I'm doing this this way is because I don't need to use the stock every time. So I thought it is better to load it when I need it. What is the best way to design this?

Edit: Some more information about the relations between the classes. I have a product class with presents a product. Each product can have a relation with the stock class.

But the stock class holds the locations (the location class contains the actual location and the amount of stock on that location). Each product can lay on different locations in the warehouse. That is way I save the locations as an array at the stock object.

In short: A product can have stock, and stock has 1 or more locations.

Answer 1

Some thoughts:

• You should never have includes inside of a class constructor like this. Your class constructor logic should exist solely within the inheritance chain for the class. If there is some kind of dependency that the class needs, inject it into the class via constructor (typically preferred) or other method. I especially find it odd that you would be including "controller" logic into a models like this.
• You have very little input validation in your public methods. You do some tyep hinting which is good, but it should be ubiquitous across your public interfaces, and for cases where you cannot type hint parameters (i.e. for certain primitives depending on target version of PHP), then include validation in your methods. There is no reason, for example that you product class properties are indeterminate in type.
• A getter-method like Product::getStock() should typically not include the actual logic to set the value. If you really need to establish this lazy loading pattern like you are using here vs. putting the stock instantiation logic in constructor, then I would at least put the stock instantiation logic in a separate function.
• I would encourage to get in the habit of using strict comparisons by default instead of the loose comparisons you seem to lean heavily on.

For example you have loose comparison in Product::getStock()

if($this->stock == null){

Which if used in other areas of code could be very problematic as this will evaluate to true for values of null, false, 0, '', [], etc. Be specific and your code will have bugs around unexpected truthy/falsey behavior.

• Why do you have get_instance() alias methods on your controller classes? Why should these controller classes have anything to do with providing instance of CI main class.
• Should you simply inject CI instances into these controllers rather than controller constructors have to hold knowledge on how to instantiate these? Do you really need to adhere to the "false singleton" way of assigning get_instance() value? Does get_instance() not take care of instance uniqueness? (sorry not overly familiar with CI).
• Stock object is immutable, so why make getTotalStock() have calculation logic need to happen on every call? Perhaps a calculateStock() method called from constructor would be best to set totalStock property. Right now you totalStock property is not even used!
• What are object contained in Stock::locations? You are clearly holding objects here based on method calls against individual object in the array. What kind of objects are these? Your code does not make this dependency clear at all.
• Core_Product_Beta::getPart() seems to present an odd interface. Have you considered splitting this into two methods, to make intent clear (i.e. getPartById() & getPartByNumber())
• return new Stock($locations, $totalStock); This does not match method signature for Stock constructor!
• Why should you need an exception to be thrown by mapProductToObject? I would think this method should not even be invoked (i.e. attempting to instantiate a Product object) if the results from $this->getInstance()->Core_Product_Model->getPartnumberFromId($partId) (and similar) call were unexpected. In fact, I don't see much reason for this method to exist as you could just do the Product instantiation in your method.

For example:

public function getPartById($partId)
{
    // not shown - validate $partId
    $productModel = $this->getInstance()->Core_Product_Model->getPartnumberFromId($partId);
    if (empty($productModel)) { // or whatever indicates failure
      return false;
    }
    return new Product(
        $productModel->id,
        $productModel->Part_number,
        $productModel->Description,
        $productModel->Type,
        $productModel->Housing,
        $productModel->Gki,
        $productModel->enabled
    );
}

• Do you have two models in your system? Your code references a Core_Product_Model, but also has Product. Perhaps you are splitting logic across two different places? Look at my code example above. Does it seem odd to you to take a perfectly good object and than pass fields from it to instantiate what is in essence an object with same sort of data? What value is Product giving you? You have the same problem it seems with Core_Stock_Model.
• Why calculate total stock in two separate places?
• Stylistically Upper_Snake_Case is seldom used in PHP code bases anymore and seems an odd departure from the camelCase used throughout the rest of your code. I would consider using UpperCamelCase for class, interface, trait, etc. names along with your camelCase method names.
• Be consistent around use of PHPDoc. Either implement it for your code or don't.

Answer 2

Your Core_Stock_Controller_Test is essentially just one function. You could easily put that logic inside the product object instead of creating a new class for just one function.

public function getStock(){
    if($this->stock == null){
        get_instance()->load->model('Core_Stock_Model');
        $result = get_instance()->Core_Stock_Model->getStockForAllLocationPerPartId($this->getId());
        $locations = array();
        $totalStock = 0;

        foreach($result as $res){
            $totalStock = $res["total"];
            $location = new Location($res["location_id"], $res["cd_mag"], $res["cd_location"], $res["cd_loc_type"], $res["stock"]);
            array_push($locations, $location);
        }

        $this->stock = new Stock($locations, $totalStock);
    }

    return $this->stock;
}