2

I have a graph service where I use if/then/else statements.

class GraphService
{
    function getGraphData(array $products)
    {
        foreach ($products as $products)
        {
            $gotPumpGraph = false;
            $model = $product['model'];
            $item = $product['model'];


            if ($model == "a" || $model == "b")
            {
                $graph = new APump();
                $graph->getGraphData($itemId);

                $graphData = $graph;
                $gotPumpGraph = true;
            }

            if ($model == "c" || $model == "d")
            {
                $graph = new CPump(new SomeDifferentDependency());
                $graph->getSomeOtherData($itemId);

                $graphData = $graph;
                $gotPumpGraph = true;
            }

            if ($gotPumpGraph)
            {
                 //put graphs into common structure
                 $graphs[] = array(
                     'itemId' => $itemId,
                     'graphData' => $graphData
                 );
            } 
        }      
    }
}

What the service is doing is consolidating various graph formats into one. Thus every if block has different dependencies and structures, that are all consolidated into one standardized structure.

The goal of my question is to find a suitable set of design patterns, to where it can hide the different dependencies found in each if block.... How?

My first thought is to use a factory but ... I see no clear path to that. The code inside if blocks is various legacy code that is different for different block and graphs come in different formats. I cleaned up the code a bit for the question, but left different methods/dependencies in the if blocks.

  • Have you considered strategy pattern? Set a list of strategies to Graphservice. For each product, look for the strategy that supports $model. Then ask to the selected one for a $graph. – Laiv Aug 8 '18 at 5:44
  • 1
    Well, it's not strategy pattern as it use to be. I'm not sure if there's a name for the pattern I'm suggesting but works similar. What I named strategies well could be named Providers APumpGraphProvider, and so on. – Laiv Aug 8 '18 at 5:52
4

I think you are correct, and would suggest a factory pattern for constructing your different 'pumps'. Each pump should have the same method: getData, which is what your main class above should call:

foreach ($products as $product)
{
    $graph = PumpFactory::forModel($product['model'];

    $graphs[] = [
        'itemId' => $product['itemId'],
        'graphData' => $graph->getData($product['model']),
    ];
}

The getData method for each pump type will of course then call the underlying correct method (getGraphData or getSomeOtherData). getData would be the method that would exist on the PumpInterface to create the contract that GraphService and other objects can depend on, with the details of how each different type of pump gets that data delegated to the individual types.

The details of the factory depend on your actual constraints, but just to show how it could be done with the code presented in the question:

class PumpFactory
{
    public static function forModel($model)
    {
        switch($model) {
            case 'a': /* FALL THROUGH */
            case 'b':
                return new APump();
            case 'c': /* FALL THROUGH */
            case 'd':
                return new CPump(new SomeDifferentDependency());
            default:
                throw new InvalidArgumentException('Invalid model: ' . $model);
        }
    }
}

My personal preference is to throw an exception in the default case, in which case you would have to handle that exception at some point. A different option is to just return null and test for it before calling getData in your loop.

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.