3
\$\begingroup\$

I am beyond stuck in a half way point between PHP structural and OOP, but I can never achieve proper SOLID patterns despite reading DI, IOC, and reading about the interfaces.

Rather than to read logical examples (i.e. a storage class that takes a book class and you typecast an interface etc...). I want to know how I can take my brain off thinking I got SOLID down but I actually didn't. There is talk about trying to refactor a small chunk but then that small chunk isn't SOLID enough.

Here are the code segments - my thought and goals are after:

class DataID
{
    protected $_id_array;
    protected $_storage;

    public function __construct(StorageInterface $db_conn, $tables = array())
    {
       $this->_storage = $db_conn;
        foreach ($tables as $table) {
            $sql = "SELECT `id` as `id`, `name` as `value` FROM $table ORDER BY `value` ASC";
            $result = $this->_storage->read($sql);
            if (!empty($result)) {
                foreach ($result as $row) {
                    $this->_id_array[$table][$row['value']] = $row['id'];
                }
            }
        }
    }
    public function getID($table, $value)
    {
        if (!empty($this->_id_array[$table])) {
            if (isset($this->_id_array[$value])) {
                return $this->_id_array[$table][$value];
            }
        }

        return null;
    }
}

Goal:

I have tables that has ID vs Value data. I'm going to use this class to run through data (value) and I want to retrieve the ID of it. Best way to avoid too many DB request is to put it inside an array of [value] = ID I believe and then check corresponding index as value and fetch ID.

Storage is an container that goes into the DB via SQL and go fetch me all the data over. This way if the method of the storage changes (Mongo, SQL, pod, MySQLi) I can change how I treat my read.

Tables is a list of known tables in the DB that I need that Data from. That list will be from an array config file that shall be loaded in a main.

Issues:

  1. I have a strong feeling that I can make this better but I don't know how - because to me a constructor initialize things and well I'm going into the DB to initialize my array but for some reason i feel that SOLID fails for me here.

  2. If I have tables that have too many values (i.e. let's say if a table look up is more than 50) I want to hit the DB and not store it inside an array.

If that's the case, I'll have to make queries inside getID to use storage to go fetch it. How do I maintain SOLID while adding that extra logic?

\$\endgroup\$
  • \$\begingroup\$ There are lots of options, but this can be a bit tricky to optimize right as it depends a bit on how the class gets used. If you are storing a lot of data, most of which never gets used, getting it all in the constructor (or elsewhere) can be expensive for little gain. Also, if you keep hitting the same few data points over and over, a more limited caching strategy might be appropriate. If you use a lot of this data, caching it all might make sense then. Two quick points I can make are that you can consolidate your SQL calls into one call, and there is a bug in getID's second if. \$\endgroup\$ – cbojar Mar 13 '14 at 17:06
  • \$\begingroup\$ @cbojar - lets assume the QUERY result itself is cached by the Storage container. They array result itself for the lookup have flexible limit in the sense that I will start with storing only 50, and anything else go fetch into db (as mentioned in #2) if need to I only have about 6 tables to do so. My goal though is not about functionality as I can probably have done a large if statement with condition and just pull everything into this class without injecting the class and just inject the Storage class. I want to know is there a better solution since I dont feel this being SOLID enough. \$\endgroup\$ – azngunit81 Mar 13 '14 at 17:12
3
\$\begingroup\$

Reading through this code and your goals, it seems this class has two responsibilities:

  • Provide a Key-Value store
  • Get data from the database (potentially with caching)

To make it more SOLID, you can pick one of these responsibilities for this class and put the other one somewhere else.

If you wanted to use this class to provide a KV store, you would inject an array pre-filled with all of the data into the constructor rather than injecting DB information. You could then use a factory that takes those parameters to build the DataID object with the data. This removes a dependency on anything database related, simplifies your constructor, and reduces the class to a thin veneer over the underlying array. It cannot, however, get new information by itself, nor can it provide database caching. You could create an adapter class around the database result set and inject that instead of the array. The result set adapter could then handle getting additional information and caching, but this might end up replicating the concerns you have here, just somewhere else.

If you wanted to use this class as an adapter over the StorageInterface class, you would not do all the work in the constructor and simply make calls to the underlying storage object in the getID method. This allows you to simplify the constructor once again, but does not in itself provide caching. You said the StorageInterface implementation provides query-level caching, but you can create a separate adapter wrapper around it that encapsulates your SQL and caches a whole table whenever one element is retrieved from it, and can limit those caches based on table size. Once again you can use a factory method so that your external application does not need to know about this internal adapter. This option seems to fit most closely with what you want to achieve while still having better SOLID-ness.

EDIT:

Here's how this achieves better adherence to the SOLID principles:

  • S: This class's responsibilities have been reduced from two to one.
  • O: I can add additional functionality by adding decorators and adapters around the various objects to add additional functionality, without having to change those objects directly.
  • L: StorageInterface uses Liskov Substitution, but an adapted class around the StorageInterface can also follow Liskov Substitution by adding decorators around it to add functionality.
  • I: The adapters are small, client-focused APIs.
  • D: StorageInterface is an abstraction, and your StorageInterface adapters/decorators can also be build as abstractions.

Bonus Points:

By removing the work out of the constructor, you are adhering more closely to the Law of Demeter. Rather than asking for something to get something through it, you are asking for what you really want.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks for a complete description of your idea. Let me digest and bring out a code to see if I've accomplished your recommendation. \$\endgroup\$ – azngunit81 Mar 13 '14 at 20:05

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.