Skip to main content
added 212 characters in body
Source Link
Laurel
  • 678
  • 4
  • 20

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

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 completely forgot $class, orfailed to check that any of the variablesthem 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).

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.

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 completely forgot $class, or to check that any of the variables 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).

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

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

Source Link
Laurel
  • 678
  • 4
  • 20

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.

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 completely forgot $class, or to check that any of the variables 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.