dimanche 18 septembre 2016

Clearly structuring testable, modular code with burgeoning business requirement conditionals

I'm trying to conceptualize the clearest way to write code for something like a Yelp or FB check-in type system, but with a check-out component, too. I'm using PHP.

The basic requirement might be: Check in: if it is type 'building', I need to

  1. look up the actual building by it's UUID to get it's primary key, which we will log with the check-in to later pull them in to reports (checked in to building '123 Main Street').
  2. they can't be currently already checked into to anything
  3. they have to be within X meters of the premises
  4. if they are not withing X meters of the premises, there must be an exception string to log to explain why.

if it is type 'room', I need to

  1. look up the actual room by it's UUID to get it's primary key, which we will log with the check-in to later pull them in to reports (checked in to '123 Main Street -> Room 212').
  2. they need to be checked into a building already
  3. they can't be checked into a room already

There is a 'check-out' phase too, which I mention so you can understand that it's not truly like Yelp where you just say "I was here". It's not a single action to just check-in; it will be followed by a 'check-out', which I won't cover here for brevity. My question is about creating cleaner controllers, methods and design patterns for structuring code that satisfies the above.

So, the request sends json, hits the (Slim) app, goes to a controller for the method (in this case, Post). We have their GPS coordinates, the UUID of the building/room they are checking into, their ID (via authentication), and the type of check-in, building or room. Something like:

{
    "latitude": "33.333", // these only matter if it is 'building'
    "longitude": "-80.343", // these only matter if it is 'building'
    "buildingUuid": "ff97f741-dba2-415a-a9e0-5a64633e13ad", // could also be 'roomUuid' ...
    "type": "building", // or 'room'
    "exceptionReason": null
}

I have all the ability to check their relationship to the building coordinates, look up the building ID, etc. I'm looking for ideas on how to make this code straightforward, maintainable and testable. For one, you can see how my switch statements (far code block) have gotten way out of hand, but I don't know how else to structure this. That's the crux of the question.

Without any requirements: (feaux code)

switch($type) {
    case 'building':
        $model = new \Namespace\Models\Building($this->container);
        break;

    case 'room':
        $model = new \Namespace\Models\Room($this->container);
        break;

    default:
        throw new \InvalidArgumentException("Type {$type} not understood");
}

$model->checkIn(); // polymorphic method that all 'type' classes will have, based on an interface implemented

I'm trying to avoid switch statements but at some point I have to use something to figure out what to instantiate, right? Polymorphism doesn't help me here, as I understand it does for a lot of switch statements, because I don't yet have an object.

So, that seems pretty straightforward, to me. What isn't clear is when I want to add some of the requirements. What is the proper way to handle this type of structure (which only involves TWO types... this could get out of hand in a hurry).

(feaux code)

switch ($type) {
    case 'building':
        $model = new \Namespace\Models\CheckInBuilding($this->container);
        $alreadyInTest = new \Namespace\Models\Buildings\BuildingTestIn($this->container);
        $building = new \Namespace\Models\Buildings\Building($this->getData('buildingUuid'), $this->container);

        if (!$building) {
            throw new Exceptions\FailBadInput('Building not found: ' . $this->getData('buildingUuid'));
        }

        $model->setBuilding($building); // building object used for these properties: ID, coordinates for the lat/long 'nearby' check
        break;

    case 'room':
        $model = new \Namespace\Models\CheckInRoom($this->container);
        $alreadyInTest = new \Namespace\Models\Rooms\RoomTestIn($this->container);
        $room = new \Namespace\Models\Rooms\Room($this->getData('roomUuid'), $this->container);

        if (!$room) {
            throw new Exceptions\FailBadInput('Room not found: ' . $this->getData('roomUuid'));
        }

        $model->setRoom($room); // room object used for these properties: ID
        break;

    default:
        throw new \InvalidArgumentException("Type {$type} not understood");
}

$model->setAlreadyInTest($alreadyInTest);
$model->setData($this->getData());
$model->checkIn();
//
// Now, for 'building|room', this has all the data internal to check:
//   - if they are nearby (but only if in building)
//   - to test if they are already logged in
//   - to log the primary key ID with the request
//

//
// In addition, and not covered here, if they are not in range of the building, they can still check-in, but need to
// type a short reason why they are doing a check-in and not in nearby range ('GPS broken, etc', who knows...). So,
// would I add yet another class, instantiated here so it can be mocked in a test, and passed in empty, so it could
// eventually be used to insert an exception reason (in a different table than the check-in table)?
//

Based on design patterns, if I need to add another $type, I just do it here, which seems like is fairly good. But... this is in a controller... it's abusing the switch statment... and it seems brittle/fragile because of all the different things that are instantiated and passed in.

I have a DIC, but I don't know that it makes things any clearer.

While it would clear up the code here, I don't want to instantiate any classes in the actual models (like, I don't want to create the 'alreadyInTester' object inside an object) because I want this to be testable and doing that seems like would make it much more difficult to test/mock.

Having said that, the tests have the same problem. There is so much riding on these various requirements and how to test them, that the tests are not very isolated. I can mock the alreadyInTest and building/room objects to isolate the checkIn method, and test building/room individually, but to test them all together, like in an integration test, now I risk non-determininstic tests because I find this to be a messy approach.

My last thought would be something like this, but I worry about fattening the controller too much: (feaux code)

switch($type) {
    case 'building':
        $alreadyInTest = new \Namespace\Models\Buildings\BuildingTestIn($this->container);
        if($alreadyInTest->isIn()) {
            throw new \InvalidArgumentException('You are already checked in to a building');
        }

        $building = new \Namespace\Models\Buildings\Building($this->getData('buildingUuid'), $this->container);

        if (!$building) {
            throw new Exceptions\FailBadInput('Building not found: ' . $this->getData('buildingUuid'));
        }

        $model = new \Namespace\Models\Building($this->container);
        break;

    case 'room':
        $alreadyInTest = new \Namespace\Models\Rooms\RoomTestIn($this->container);

        if($alreadyInTest->isIn()) {
            throw new \InvalidArgumentException('You are already checked in to a room');
        }

        $room = new \Namespace\Models\Rooms\Room($this->getData('roomUuid'), $this->container);

        if (!$room) {
            throw new Exceptions\FailBadInput('Room not found: ' . $this->getData('roomUuid'));
        }

        $model = new \Namespace\Models\Room($this->container);
        break;

    default:
        throw new \InvalidArgumentException("Type {$type} not understood");
}

$model->setData($this->getData());
$model->checkIn();

Again, I feel like I should be abstracting those two if/throws (per case) elsewhere, but that doesn't seem to make this any more straightforward (also: I admit this isn't a complicated example... yet), and the controller isn't any skinnier in either example. I feel this last example is more clear, to me. The crux, to me, is that each time something would be added, it would mean adding more to the switch statement. I would think a polymorphic system would be better, but since there would be outside classes needed to do the requirement checking, that I'd have to instantiate and pass a ton of objects in anyway, making it just as complex. Instantiating classes inside each check-in object is not as testable. I was thinking maybe Chain of Responsibility or Command patterns might be useful, but don't seem to be quite a true fit.

I go round and round.

So, is one of these the best way, or is there something I could be doing better?

Aucun commentaire:

Enregistrer un commentaire