Custom Collection Refactoring Question

Hey everyone,

Looking for an opinion. I've been working on a shipment project where I need to post tracking numbers to our eCommerce site. The only issue is that shipments come from 3 different sources completely (API, DB2 (ugh!), flat file). I've been refactoring a good bit of the code to try and transform all the shipments to a common format. Also to a way that I could typehint all the methods that accept a shipment/shipments as a parameter.

Here's what I've come up with so far: I decided that I would extend Collections so that I could map the shipments to a custom collection to make it consistent from all three data sources. This way, I can typehint and methods that take shipments as a parameter e.g. $shopify->addShipments($order, $shipments) into $shopify->addShipments(Order $order, ShipmentCollection $shipments). What was happening before was that each class that has a getShipment call would transform the data into an associative array.

As you can see what I was trying to do here is take the results of any source of data and run it through ShipmentCollection::make($results); to have it return a custom collection mapped to a specific format. The only thing that sucks/bothering me is having this nasty conditional statement because the data is so inconsistent coming from the other sources.

Does anyone have a suggestion on how to make this better? I keep thinking to myself that I'm not sure if this is the best way to do this and I'm not sure if using collections is the best way to achieve this. Appreciate the time. Cheers.

<?php
namespace App\Collections;

use Illuminate\Support\Collection;

class ShipmentCollection extends Collection
{

    public static function make($items = [])
    {
        $items = collect($items)->map(function ($shipment) {
            return [
                'trackingNumber' => self::trackingNumber($shipment)
            ];
        });

        return new static($items);
    }

    private static function trackingNumber($shipment)
    {
        if (isset($shipment['TrackingNumber'])) {
            return $shipment['TrackingNumber']; // flat file
        } elseif (isset($shipment['SDDSC1'])) {
            return rtrim($shipment['SDDSC1']); // DB2
        } elseif (isset($shipment->trackingNumber)) {
            return $shipment->trackingNumber; // API
        }

        return null;
    }
}

FrancescoZaffaroni

9 years ago

Best Answer

Level 6

You could implement the strategy pattern, and set the strategy when you call shipmentCollection::make(). for example

ShipmentCollection::make($resultsFromFlatFile, FlatFileStrategy::class);

ShipmentCollection::make($resultsFromDB2, DB2Strategy::class);

ShipmentCollection::make($resultsFromAPI, APIStrategy::class);

the make() method would accept a second parameter called strategy and pass it to the trackingNumber method.


interface IStrategy {

    public function getTrackingNumber($shipment);

}

class FlatFileStrategy  implements IStrategy {

    public function getTrackingNumber($shipment)
    {
        return $shipment['TrackingNumber'];
    }
}

class APIStrategy  implements IStrategy  {

    public function getTrackingNumber($shipment)
    {
        return $shipment->trackingNumber;
    }
}

class DB2Strategy  implements IStrategy  {

    public function getTrackingNumber($shipment)
    {
        return $shipment['SDDSC1'];
    }
}

and then in the trackingNumber method of the collection would be:

 private function trackingNumber($shipment, $strategy)
{
    return (new $strategy)->getTrackingNumber($shipment);
}

if you want to remove the if-else but I believe it's a bit overkill. I believe it's all right to have that if else.

Note: You can also instantiate the strategy when you call Collection::make() and pass it as a parameter instead of instantiating it in Collection::trackingNumber() and it would probably be better because you are injecting the dependency, but it wouldn't make that big of a difference.

Note 2: This would work with the assumption you are creating the collections separately for every data source and not merging them before.

1 like

Please or to participate in this conversation.