1
\$\begingroup\$

I'm starting to code in the OOP way, but I have doubts with the following example. I have a Product entity and I want to import & create instances of this entity from a CSV file. I load the CSV file into an array in another part of the code, then I pass the array with the data to CSVImport class.

class Product
{
    private $id;

    private $name;

    private $price;

    private $stock;

    private $imageUrl;

    public function __construct() {}

    /** Getters & setters */
}

class CSVImport extends ProductImport
{
    private $websiteUrl;

    public function __construct($websiteUrl)
    {
        $this->websiteUrl = $websiteUrl;
    }

    public function import($csvData)
    {
        $productCollection = new ProductCollection();

        foreach ($csvData as $rowData) {
            $product = $this->parseProductData($rowData);
            $productCollection->addProduct($product);
        }

        return $productCollection;
    }

    private function parseProductData($productData)
    {
        $product = new Product();

        list(
            $id,
            $name,
            $price,
            $stock,
            $imagePath
        ) = $productData;

        $product
            ->setId($id)
            ->setName($name)
            ->setPrice($price)
            ->setStock($stock)
            ->setImageUrl($this->websiteUrl.$imagePath);

        return $product;
    }
}

My questions are:

  1. Is it correct to pass an array of data to CSVImport class? Or should it take the responsability to read & load the CSV file?
  2. Who should take care of data validation, Product setters, CSVImport class or a new class? And how can I manage validation errors?
  3. In case I need to transform the source data, as I've done with imageUrl, is the correct place to do it?
\$\endgroup\$
5
  • \$\begingroup\$ Welcome to Code Review. Your comment /** Getters & setters */ and the ProductCollection indicates that there is some code missing. We expect your code to include all parts necessary for a review. \$\endgroup\$ – Zeta Mar 12 '19 at 14:57
  • \$\begingroup\$ Hello @Zeta, thank you for your comment, but I was trying not to post a large amount of code. I think that the code can be understood without those parts. \$\endgroup\$ – David Mar 13 '19 at 12:24
  • \$\begingroup\$ This is my Opinion, 1. CSVImport , unless you also need to do arrays, in which case make another method for reading the data from the file, but it should be part of the class. The class won't work well without the CSV data. You'd have to make changes to both if fields are added etc.. You can also remove a loop by importing where the foreach is and using the file reading loop instead. 2 Product setters, if you ever need to manually create a product, you want them to do the validation too. 3 Should be part of validation, bad data goes in set, good data comes from get \$\endgroup\$ – ArtisticPhoenix Mar 15 '19 at 10:32
  • \$\begingroup\$ For errors, that's harder to say. Use exceptions for validation errors, catch them in the CSV reading loop, so you can continue reading. You'll have to decide what state to leave the Product class in if you do a partial read. If you don't mind passing the constructor an array of this data, even an optional array, you could do all the setting there $this->{"set$key"}($value) etc. But the big advantage is throwing an exception before the constructor completes destroys the object. Because the constructor can't return the instance in that case. \$\endgroup\$ – ArtisticPhoenix Mar 15 '19 at 10:38
  • \$\begingroup\$ Sorry, this $this->websiteUrl.$imagePath is part of the input. You can't/shouldn't pass it separatly to the Product \$\endgroup\$ – ArtisticPhoenix Mar 15 '19 at 10:44
1
\$\begingroup\$

As I said in the comments:

For errors, that's harder to say. Use exceptions for validation errors, catch them in the CSV reading loop, so you can continue reading. You'll have to decide what state to leave the Product class in if you do a partial read. If you don't mind passing the constructor an array of this data, even an optional array, you could do all the setting there $this->{"set$key"}($value) etc. But the big advantage is throwing an exception before the constructor completes destroys the object. Because the constructor can't return the instance in that case

Something like this

class Product
{

    private $imageUrl = false;

    public function __construct(array $data = []){
          if(!empty($data)){
             foreach($data as $line=>$value){
                if(isset($this->{$key})){
                  $this->{"set$key()"}($value);
                }
             }
         }
    }

    public setImageUrl($image){
          if(empty($image)) throw new InvalidArgumentException("Image is empty");
           $this->imageUrl= $image;

    }
}

There are a few tricks here, but you have to set the default to false for if(isset($this->{$key})) to work, but it's faster then property_exists. This part will dynamically call setimageUrl in this case. The lowercase i doesn't matter much in setImageUrl as user defined methods/function are case insensitive.

The other part I said is this:

This is my Opinion, 1. CSVImport , unless you also need to do arrays, in which case make another method for reading the data from the file, but it should be part of the class. The class won't work well without the CSV data. You'd have to make changes to both if fields are added etc.. You can also remove a loop by importing where the foreach is and using the file reading loop instead. 2. Product setters, if you ever need to manually create a product, you want them to do the validation too. 3. Should be part of validation, bad data goes in set, good data comes from get

Most of that is pretty self explanatory. For #3 the ->setImageUrl($this->websiteUrl.$imagePath); is part of the input data. So that is fine. You don't want to have to pass them separately. In fact with data loading in the constructor of the Product you can skip that method completely as it becomes:

 private function parseProductData($productData)
 {
     return new Product($productData);
 }

So that alone saves you a call to list and a call to parseProductData for each product.

I can give you more examples if you need them

I would do the CVSImport something like this (untested)

class CSVImport extends ProductImport
{
    private $websiteUrl;

    private $errors = [];

    public function __construct($websiteUrl)
    {
        $this->websiteUrl = $websiteUrl;
    }

    public function import($csvData)
    {
        $productCollection = new ProductCollection();
         //you could do file reading here with a while loop
        foreach ($csvData as $line=>$rowData) {
            try{
                $productCollection->addProduct(new Product($rowData));
            }catch(Exception $e){
                $this->errors[$line] = get_class($e)."::{$e->getCode()} {$e->getMessage()} in {$e->getFile()} on {$e->getLine()}";
            } 

        }

        return $productCollection;
    }

    //or for max flexabillity you can do it here
    public function readCsv($filename){
        if(!file_exists($filename)) throw new Exception("Could not find file $filename".);//etc.

        $f = fopen($filename, 'r');

        $header = fgetcsv($f); //get header row
        $len = count($header);

        $line = 0;
        $rowData= [];
        while(!feof($f)){
            ++$line;

            try{
                $current = fgetcsv($f);

                if(count($current) != $len) throw new Exception("Delimter count missmatch on line $line of $filname");

                $rowData[$line] = array_combine($header, $current); //combine [$haders => $current]
            }catch(Exception $e){
                $this->errors[$line] = get_class($e)."::{$e->getCode()} {$e->getMessage()} in {$e->getFile()} on {$e->getLine()}";
            } 
        } 

        return $rowData;
    }

    public function getErrors(){
        return $this->errors;
    }

    public function hasErrors(){
         return count($this->errors)
    }

}

So you see the try/catch is almost the same, you could combine these two methods readCsv and import. But then you cannot accept array as data only the file. That's up to you.

The basics are that you can do new Product($data) and it does all the setting of the data using it's set methods. Then if you throw exceptions in those set methods it not only helps you when using them manually, but when constructing a product from the CSV, the product dies as the exception is thrown. Then we catch that, keep track of it, and move on to the next row. Each part handling it's own concerns.

In any case now it's all nice and clean, the other option would be to make a class just for reading CSV data, that takes headers as an argument, and does some mapping on them etc. But its a lot easier to find then putting the CSV stuff in some random place.

Cheers.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.