We use cookies on this site to enhance your user experience. By clicking "OK, I Agree" or using our site, you consent to the use of cookies unless you have disabled them.
View our cookie policy to learn more.

To get our ships we use ShipLoader which queries the database
and creates ship objects. This queryForShips() goes out, selects all
the ships, and then later it is passed to this nice createShipFromData()
function down here:

Now why would we want our application to sometimes load from the database and other times from a
JSON file? Say that when we're developing locally we don't have access to our database,
so we use a JSON file. But when we push to production we'll switch back to the
real database. Or, suppose that our ship library is so awesome that someone else wants
to reuse it. However, this fan doesn't use a database, they only load them from
JSON.

This leaves us needing to make our ShipLoader more generic.

Right now, all of the logic of querying things from the database is hardcoded in here.
So let's create a new class whose only job is to load ship data through the database, or PDO.

Sometimes we query for all of the ships and sometimes we query for just one ship by ID.

Back to our PdoShipStorage I'll create two methods, to cover both of those actions. First,
create a public function fetchAllShipsData() which we'll fill out in just one second. Now,
add public function fetchSingleShipData() and pass it the id that we want to query for:

Notice, that we're not returning an object here this is just a really dumb class
that returns data, an array in our case.

There is one problem, we have a getPdo() function inside of ShipLoader that references a pdo
property. Point being, our PDO storage needs access to the PDO object, so we're going to use
dependency injection, a topic we covered a lot in
episode 2 . Add
public function __construct(PDO $pdo) and store it as a property with
$this->pdo = $pdo;:

Now we have a class whose only job is to query for ship stuff, we're not using it anywhere yet, but it's fully
ready to go. So let's use this inside of ShipLoader instead of the PDO object. Since we don't need PDO to be
passed anymore swap that out for a PdoShipStorage object. Let's update that in a few other places and change
the property to be called shipStorage:

Perfect, now scroll down and get rid of the queryForShips() function all together:
we're not using that anymore. And while we're cleaning things out also delete this getPDO() function.
We can delete this because up here where we reference it in findOneById() we'll do the same thing.
Remove all the pdo querying logic, and instead say shipArray = $this->shipStorage->fetchSingleShipData();
and pass it the ID:

All we know is that we're passed in some PdoShipStorage object and we're able to call methods on it. It can
make the queries and talk to whatever database it wants to, that's it's responsibility. In here we're just
calling methods instead of actually querying for things.

ShipLoader and PdoShipStorage are now fully setup and functional. The last step is going into our container
which is responsible for creating all of our objects to make a couple of changes. For example, when we
have new ShipLoader we don't want to pass a pdo object anymore we want to pass in PdoShipStorage.

Just like before, create a new function called getShipStorage() and make sure we have our property up above.
The getShipStorage() method is going to do exactly what you expect it to do. Instantiate a new PdoShipStorage
and return it. The ship's storage class does need PDO as its first constructor argument which we do with
new PdoShipStorage($this->getPDO());:

Everything used to be in ShipLoader, including the query logic. We've now split things up so that the query
logic is in PdoShipStorage and in ShipLoader you're just calling methods on the shipStorage. Its real
job is to create the objects from the data, wherever that data came from. In Container.php we've wired
all this stuff up.

Phew, that was a lot of coding we just did, but when we go to the browser and refresh,
everything still works exactly the same as before. That was a lot of internal refactoring.
In index.php as always we still have $shipLoader->getShips():

And that function still works as it did before, but the logic is now separated into two pieces.

The cool thing about this is that our classes are now more focused and broken into smaller pieces. Initially
we didn't need to do this, but once we had the new requirement of needing to load ships from a JSON file this
refactoring became necessary. Now let's see how to actually load things from JSON instead of PDO.

Leave a comment!

2019-07-19Victor Bocharsky

Hey Dirk,

No problem! ;) I know, some of our challenges might be tricky, at least at the first sight. Thank you for reporting a problem anyway, sometimes it might be valid, so better to double check

Cheers!

2019-07-18Dirk J. Faber

whoopsie, you are right, Cheers!

2019-07-16Victor Bocharsky

Hey Dirk,

Well, you're wrong :) It cannot be if ($result === $this->cache->fetchFromCache($str)) because $result var is not defined. Actually, we define it in that if for the first time. You can think of the code:

Ah, love the question and seeing your approach :). So in general, of course, if you're avoiding the code duplication - through whatever means - that's best. But, there are a few subtle downsides to using the property versus the private method, at least in this case:

1) Where/When is the $cacheFile property set? Right now, you're setting it in fetchFromCache(). So technically, if someone uses the Cache class in the future and calls saveToCache() first, we'll have a problem.

2) When you have these "service" or "worker" classes, they're meant to be built in a way that makes them reusable over and over again. I mean, like you should be able to use the Cache object to fetch/store many different strings. But when you set the $cacheFile as a property, this is actually the $cacheFile for *one* specific string (since it contains the md5). That's a problem :). Your Cache class should only contain properties (sometimes called "state" or configuration) that describe the behavior of the class *itself*: its properties/state shouldn't start containing configuration for specific instances of it being used, else it gets weird if you re-use it. So, a cacheFile, which is specific to a single cache key is wrong. But, you *could* decide to have a $cacheDir property, which is the *directory* that *all* cache files will be saved in. It's a little unnecessary here, but you could create a __construct() method and set that property inside of there. That property is configuration for the Cache object itself ("where do I store cache files"). If you made your cache class even smarter and started making cached items "expire"after some amount of time, then another good example might be a `$defaultExpirationLength` property, which would be how long each cache file should exist (by default, because perhaps you allow the user to override this via a 3rd parameter to saveToCache) before we consider it expired. That's another good property of how the Cache object works in general. Btw, this topic relates heavily to the topic on "Service classes" (a big topic in part 2 http://knpuniversity.com/sc.... Cache is a service class, so if it has *any* properties, they should only be configuration for how that class works. But, Ship (from the screencast), for example, is a *model* class, whose job is to store data. For those types of classes, the properties represent the data for that *one* object. That's important because, when done nicely, properties have somewhat different jobs in the two types of classes.

So the tl;dr would be: only use a property (instead of a method) if the item in question is really configuration or data for the object as a whole. Phew!

And about your parameter/argument names, yea, I think your instinct is correct :). When you're coding inside, for example, the Cache class, try to imagine that the Cache class is your entire world and nothing else exists. Then ask, "how should I name this parameter?". Of course, the answer will be to name it in a way that describes how it's used inside *that* class. Other devs - or future you! - will thank you in the future: the variable names become a bit of "free" documentation about what those arguments mean.

Thanks for the great question! Cheers!

2016-10-07PlayIt

Hey, I came up with a slightly different result in the first challenge and was wondering if there is anything wrong with the way I did it. The main reason I'm following this coarse is to brush up on things and learn how to structure my code in a way that will mesh with other developers down the road..

As you can see I avoided the code duplication too, but instead of using a private method, I used a private property. Seems that this would be more performant in this particular caching example at the expense of code readability and separation.

The questions I'm left with are: Am I missing something or or wrong for that approach? After reviewing both, which would you choose to use in production and why? Also, I have a habit of changing the parameter names to match the variable names passed in, I think I see why that's bad now, as it seems obvious that the parameter names of the method should match their use in the method.. Am I missing anything not so obvious here?