Refactoring this code into OOP & less functional programming

Question

I got feedback from an interview test where they wanted me to prove that I've got a grasp of OOP php programming. This file especially I got feedback on that it's more functional than it is Object-Oriented so I'm asking all the PHP gurus if they have any tips on how I can turn this into more of an OO component.

The first file:

<?php

require_once('includes/initialize.php');

require_once('productClass.php');

header('Content-Type: application/json; charset=UTF-8');



$response = array();

if (isset($_POST['sku']) && isset($_POST['name']) && isset($_POST['price']) && isset($_POST['type']) && isset($_POST['description'])) {
    $class = $_POST['type'];
    $product = new $class($_POST['sku'], $_POST['name'], $_POST['price'], $_POST['type'], $_POST['description']);

    $sku = $product->formatSKU($product->getType(), $product->getSKU());
    $name = $product->getName();
    $price = $product->getPrice();
    $type = $product->getType();
    $description = $product->formatDescription($product->getDesc());

    $result = $api->addProduct($sku, $name, $price, $type, $description);

    if ($result) {
        header("HTTP/1.1 200 OK");
        header("Location: https://scandiweb-product-page.herokuapp.com/");
        exit();
    } else {
        header("HTTP/1.1 406 Error inserting product");
    }
} else {
    header("HTTP/1.1 499 Required parameters missing");
    echo 'Required parameters missing';
}

This is the product class file:

<?php

abstract class Product
{
    private $prodSku;
    private $prodName;
    private $prodPrice;
    private $prodType;
    private $prodDesc;

    public function __construct($prodSku, $prodName, $prodPrice, $prodType, $prodDesc)
    {
        $this->prodSku = $prodSku;
        $this->prodName = $prodName;
        $this->prodPrice = $prodPrice;
        $this->prodType = $prodType;
        $this->prodDesc = $prodDesc;
    }

    public function getSKU()
    {
        return $this->prodSku;
    }
    public function getName()
    {
        return $this->prodName;
    }
    public function getPrice()
    {
        return $this->prodPrice;
    }
    public function getType()
    {
        return $this->prodType;
    }
    public function getDesc()
    {
        return $this->prodDesc;
    }

    public function formatSKU($inputType, $inputSKU)
    {
        return strtoupper(substr($inputType, 0, 4)) . str_pad($inputSKU, 4, "0", STR_PAD_LEFT);
    }

    abstract protected function formatDescription($inputDesc);
}

class DVD extends Product
{
    public function formatDescription($inputDesc)
    {
        return "Size: " . $inputDesc . "MB";
    }
}
class Furniture extends Product
{
    public function formatDescription($inputDesc)
    {
        $implodedInput = implode('x', $inputDesc);
        return "Dimensions: " . $implodedInput;
    }
}
class Book extends Product
{
    public function formatDescription($inputDesc)
    {
        return "Weight: " . $inputDesc . "kg";
    }
}

Their feedback was I needed to utilize getters & setters, any idea how can this be achieved? any help is appreciated.

Answer 1

Product and subclasses

Your code is old fashioned and easy to misuse due to the problems you have with OOP. Consider this, which you're letting me do with your code:

$dvd = new DVD("12345", "Your Name", "15 dollars", "Romance", "35MB");

echo($dvd->formatDescription("Not the description"));

This shows that it's easy to make a lot of mistakes. I'm echoing out something that's not the description (an OOP problem), and several variables with an incorrect format (which can be fixed via modern PHP).

Here's a quick rewrite of the Product and DVD classes:

abstract class Product
{
    public function __construct(
        private readonly string $sku, 
        public readonly string $name,
        public readonly int $price, 
        public readonly string $type, 
        protected readonly int|array $description
    ){}

    public function formatSKU(): string
    {
        return strtoupper(substr($this->type, 0, 4)) . str_pad($this->sku, 4, "0", STR_PAD_LEFT);
    }

    abstract protected function formatDescription(): string;
}

class DVD extends Product
{
    public function formatDescription(): string
    {
        return "Size: " . $this->description . "MB";
    }
}

// Test it:
$dvd = new DVD("12345", "Your Name", 1500, "Romance", 35);
echo($dvd->formatSKU() . '    ' . $dvd->formatDescription());

Some highlights:

• Used $this->variableName instead of redundantly and possibly mistakenly passing in the variable from outside the class
• Constructor promotion (PHP 8.0)
• Readonly properties (PHP 8.1), which replace your getters while not allowing accidental overwrites. (You don't ever re-set the variable so why would you need setters?)
• Type declarations and return declarations (not even that new)
• Renamed some variables to be more clear ($description) or less redundant (the Product's prod variables).

The $description being the union type (PHP 8.0) "int|array" shows how weird your design is for this particular variable (since it is an array of two ints for the furniture class and an int everywhere else), so that's something I'd also refactor.

I'm also not so sure about the $type... can it really be any string? Maybe you could use an enum (or several) from PHP 8.1 to lock that down to the only types that should exist. Since all members of a class will always have the same type, it doesn't make sense to pass this in; instead you could make an abstract method on Product return this, and then implement it differently inside each subclass for each value (iirc).

Main file

What really sticks out to me here is the lack of validation. You made an attempt to ensure that all the POST variables were set but failed to check that any of them contained what you wanted (e.g. a positive int for the price, in cents — which would avoid the possibility of floating point errors). You're assuming that your frontend is going to pull all the weight, but it can't: a user can submit a POST request with whatever they want. Especially after the rewrite you need to validate, since the outcome otherwise will be a server error. Especially make sure that $class is one of the classes you want (which I would use match from PHP 8 for).

Also $api->addProduct($sku, $name, $price, $type, $description); should probably accept a Product instead, if you have any control over that.