Buy

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:

61 lines lib/Service/ShipLoader.php
... lines 1 - 2
class ShipLoader
{
... lines 5 - 14
public function getShips()
{
$ships = array();
$shipsData = $this->queryForShips();
foreach ($shipsData as $shipData) {
$ships[] = $this->createShipFromData($shipData);
}
return $ships;
}
... lines 27 - 58
}
... lines 60 - 61

This is the one we've been working in that creates the objects.

  • Step 1: Query the database
  • Step 2: Turn that data into objects

Suppose that we have a new requirement, sometimes we're going to get the ship data from the database but other times it will come from a different source, like a JSON file.

In the resources directory there's a new ship.json file, as you can see this holds the same info as we have in the database:

35 lines resources/ships.json
... line 1
[
{
"id": "1",
"name": "Jedi Starfighter",
"weapon_power": "5",
"jedi_factor": "15",
"strength": "30",
"team": "rebel"
},
... lines 11 - 26
{
"id": "4",
"name": "RZ-1 A-wing interceptor",
"weapon_power": "4",
"jedi_factor": "4",
"strength": "50",
"team": "empire"
}
]

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.

Create a new class called PdoShipStorage:

33 lines lib/Service/PdoShipStorage.php
... lines 1 - 2
class PdoShipStorage
{
... lines 5 - 31
}

Looking back inside ShipLoader there are two types of queries that we make:

79 lines lib/Service/ShipLoader.php
... lines 1 - 2
class ShipLoader
{
... lines 5 - 31
public function findOneById($id)
{
$statement = $this->getPDO()->prepare('SELECT * FROM ship WHERE id = :id');
... lines 35 - 42
}
... lines 45 - 68
private function queryForShips()
{
$statement = $this->getPDO()->prepare('SELECT * FROM ship');
... lines 72 - 75
}
}
... lines 78 - 79

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:

33 lines lib/Service/PdoShipStorage.php
... lines 1 - 2
class PdoShipStorage
{
... lines 5 - 11
public function fetchAllShipsData()
{
... lines 14 - 17
}
public function fetchSingleShipData($id)
{
... lines 22 - 30
}
}

Before we go any further head back to our bootstrap.php file and make sure that we require this:

19 lines bootstrap.php
... lines 1 - 14
require_once __DIR__.'/lib/Service/PdoShipStorage.php';
... lines 16 - 19

Perfect!

What I want to do is move all the querying logic from ShipLoader into this PdoShipStorage class. Let's start with the logic that queries for one ship and pasting that over here:

33 lines lib/Service/PdoShipStorage.php
... lines 1 - 2
class PdoShipStorage
{
... lines 5 - 19
public function fetchSingleShipData($id)
{
$statement = $this->pdo->prepare('SELECT * FROM ship WHERE id = :id');
$statement->execute(array('id' => $id));
$shipArray = $statement->fetch(PDO::FETCH_ASSOC);
if (!$shipArray) {
return null;
}
return $shipArray;
}
}

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

33 lines lib/Service/PdoShipStorage.php
... lines 1 - 2
class PdoShipStorage
{
private $pdo;
public function __construct(PDO $pdo)
{
$this->pdo = $pdo;
}
... lines 11 - 31
}

If this pattern is new to you just head back and watch the dependency injection video in episode 2 of the OO series.

Here we're saying, whomever creates our PDO ship storage class must pass in the pdo object. This is cool because we need it. Now I can just reference the property there directly.

Back in ShipLoader copy the entire queryForShips() and paste that into fetchAllShipsData() and once again reference the pdo property:

33 lines lib/Service/PdoShipStorage.php
... lines 1 - 2
class PdoShipStorage
{
... lines 5 - 11
public function fetchAllShipsData()
{
$statement = $this->pdo->prepare('SELECT * FROM ship');
$statement->execute();
return $statement->fetchAll(PDO::FETCH_ASSOC);
}
... lines 19 - 31
}

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:

61 lines lib/Service/ShipLoader.php
... lines 1 - 2
class ShipLoader
{
private $shipStorage;
public function __construct(PdoShipStorage $shipStorage)
{
$this->shipStorage = $shipStorage;
}
... lines 11 - 58
}
... lines 60 - 61

Cool!

Down in getShips() we used to call $this->queryForShips(); but we don't need to do that anymore! Instead, say $this->shipStorage->fetchAllShipsData();:

61 lines lib/Service/ShipLoader.php
... lines 1 - 2
class ShipLoader
{
... lines 5 - 54
private function queryForShips()
{
return $this->shipStorage->fetchAllShipsData();
}
}
... lines 60 - 61

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:

61 lines lib/Service/ShipLoader.php
... lines 1 - 2
class ShipLoader
{
... lines 5 - 31
public function findOneById($id)
{
$shipArray = $this->shipStorage->fetchSingleShipData($id);
... lines 35 - 36
}
... lines 38 - 58
}
... lines 60 - 61

This class now has no query logic anywhere.

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());:

71 lines lib/Service/Container.php
... lines 1 - 2
class Container
{
... lines 5 - 12
private $shipStorage;
... lines 15 - 49
public function getShipStorage()
{
if ($this->shipStorage === null) {
$this->shipStorage = new PdoShipStorage($this->getPDO());
}
return $this->shipStorage;
}
... lines 58 - 69
}

Up in getShipLoader(), now pass $this->getShipStorage():

71 lines lib/Service/Container.php
... lines 1 - 2
class Container
{
... lines 5 - 40
public function getShipLoader()
{
if ($this->shipLoader === null) {
$this->shipLoader = new ShipLoader($this->getShipStorage());
}
... lines 46 - 47
}
... lines 49 - 69
}

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():

126 lines index.php
... lines 1 - 6
$ships = $shipLoader->getShips();
... lines 8 - 126

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!

  • 2016-10-09 PlayIt

    Future me will thank you for saving me from future headaches.

    .. Thanks!

    See, I knew it would happen.

  • 2016-10-08 weaverryan

    Hey PlayIt!

    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/scree.... 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-07 PlayIt

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

    Here's a link to the code: http://paste.ofcode.org/DknvxY...

    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?

    Thanks for the great track!