A find method on a Product class

Question

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.

Answer 1

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

Answer 2

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;
    }
}