2
\$\begingroup\$

The following class holds an array with products that are generated from a Json file. This is what the json file looks like

[
  {
    "id": "A101",
    "description": "Screwdriver",
    "category": "1",
    "price": "9.75"
  },
  {
    "id": "A102",
    "description": "Electric screwdriver",
    "category": "1",
    "price": "49.50"
  },
  {
    "id": "B101",
    "description": "Basic on-off switch",
    "category": "2",
    "price": "4.99"
  }
]

I'm using a find method on a Product class, but I'm in doubt if storing all generated products in the class itself is a good idea

  • Should I maybe create a ProductCollection class?
  • Should I build my products[] collection differently then using my method
  • Should I generate a Factory for the product creation from my Json file.

This is my code

    class Product {

        private static $products = [];

        public static function initialize($url)
        {
            // Build the product from our JSON file
            $data = json_decode(file_get_contents($url));
            foreach($data as $item) { 
                $product = new Product();
                $product->id = $item->id;
                $product->description = $item->description;
                $product->category = $item->category;
                $product->price = $item->price;

              self::$products[] = $product;
            }
        }

        public static function find($id)
        {
            foreach(self::$products as $product) {
                if($product->id == $id) {
                    return $product;
                }
            }

            return false;
        }
    }

Thank you.

\$\endgroup\$
2
\$\begingroup\$

Look closely at your code. What's the major difference between what you have now and this evil snippet:

$PRODUCTS = [];
function loadProducts($url)
{
    global $PRODUCTS;
    $data = json_decode(file_get_contents($url));
    foreach ($data as $item) {
        $PRODUCTS[] = new Product($item);//assuming __construct(stdClass $data)
    }
}

function findProduct($id)
{
    global $PRODUCT;
    if (isset($PRODUCT[$id])) {
        return $PRODUCT[$id];
    }
    return false;
}

There isn't much of a difference, is there? This is why static's in PHP especially, are often referred to as being globals in drag.
If the $PRODUCTS array were public in your code, or a true global (as is the case in my horrible snippet, why would I bother calling a method, instead of just inlining the $product = isset($PRODUCTS[$id]) ? $PRODUCTS[$id] : null;?

You're essentially tightly coupling functionality and state, which is never a good idea. You're offering no clean way to the user to fetch a certain data-set without that data being available to all parts of the application. A cleaner thing to do would be to have some sort of "fetcher" component:

class JsonUrlReader
{//horrible name, but it's late and this is what came up first
    protected $currentData = [];//for buffered reads

    /**
     * Allow user to pass URL + the class you want the data to mapped on to
     * @param string $url
     * @param string|null $class
     */
    public function fetchData($url, $class = null)
    {
        $data = json_decode(file_get_contents($urls));
        $items = [];
        foreach ($data as $item) {
            //pass data to constructor if class arg is given
            $item = $class ? new $class($item) : $item;
            $this->currentData[] = $items[] = $item;
        }
        return $items;//return current subset
    }
}

You could define interfaces that allow you to index the return array, you can add methods to fetch the $currentData property in full, or perhaps clear it.
You could extend this class to fetch the data in a more specific way, or to focus around a known API (eg a wrapper for a specific api can have methods that make predefined, known calls and return known objects, but behind it all, they just perform curl calls).

Another problem you will face in time is testing. As it stands, your code is pretty much un-testable.
The only thing you can do is pass files to the initialize method, that contain known sets of data, but you're not checking the json_last_error value. At no point are you considering malformed JSON, timeouts or worse still: invalid arguments (what if I pass null or false?).

You're assuming the caller will pass valid arguments at all time, you're also assuming the file_get_contents call will be successful, and the contents will be valid JSON.
What's more, you're assuming that this valid JSON will be decoded into objects of a particular format.

Ask yourself this simple question: if the third party providing the data changes their format, what would make most sense: you having to edit all places where you're setting the data transfer objects (eg Product), or you having to change the Product object itself (the thing that represents the third party data)?

This is about all I have time for now, will update if I can...

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your great answer. I made a JsonParser class which handles over an array with stdobjects in it.. Any idea where I should store all the products? I need to be able to pass the whole collection of objects to other methods.. I tried making a repository. But this code doesn't feel right, I uploaded a screenshot ( If you have 2minutes of time) thank you! prntscr.com/axfpqt \$\endgroup\$ – Notflip Apr 27 '16 at 11:22
  • 1
    \$\begingroup\$ @Notflip: Having a parser class seems like a reasonable thing to do. If you need to pass around an array of products, there's nothing wrong with passing just an array, unless you're grouping the products by type. If that's the case, you could use a traversable object that holds an array of product instances, and a property to indicate what type of products it holds. I briefly looked at your screenshot, but to me, a repository is linked with storage (DB or whatever), you just have a collection object. I'll look into it after work though \$\endgroup\$ – Elias Van Ootegem Apr 27 '16 at 12:44
  • 1
    \$\begingroup\$ Thank you for your comment! The problem is I'm using products.json and quote.json, making a parser class that can handle both and traverse into nested json seems a bit difficult for the moment. Maybe I can make a parser class specific for the class it's parsing for? ProductParser and QuoteParser, and they return an array with all items parsed from it's url. What do you think? Oh and thank you so much! \$\endgroup\$ – Notflip Apr 27 '16 at 12:47
  • 2
    \$\begingroup\$ @Notflip: On the face of it, creating separate classes that share 50% of the functionality seems a bit wasteful. Abstract away what you can (ie loading the JSON, parsing, and checking/handling any errors that might occur). Then extend that class so that the child classes use the parents' methods to load the data, and in the child class, handle the expected format accordingly (ie the child classes are type-specific) \$\endgroup\$ – Elias Van Ootegem Apr 27 '16 at 13:27
2
\$\begingroup\$

Use indexes on products

class Product {

    private static $products = [];

    public static function initialize($url)
    {
        // Build the product from our JSON file
        $data = json_decode(file_get_contents($url));
        foreach($data as $item) { 
            $product = new Product();
            $product->id = $item->id;
            $product->description = $item->description;
            $product->category = $item->category;
            $product->price = $item->price;

          self::$products[$product->id] = $product;
        }
    }

    public static function find($id)
    {
        if(isset(self::$products[$id])) {
            return self::$products[$id];
        }

        return false;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please expand a bit more about the "why" rationale, and this will be an "ok" CR answer. The best answers typically review OP's code - "try this!" answers are actively discouraged on this site. \$\endgroup\$ – Mathieu Guindon Apr 26 '16 at 16:39

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.