Create a pump object from either one of pump model number or pump product id

Question

Consider this code:

class AssemblyFactory
{
    public function makeAssembly(array $params)
    {
        if (array_key_exists('modelNumber', $params))
            $pump = $pumpFactory->fromModelNumber($params['modelNumber']);
        else if (array_key_exists('productId', $params))
            $pump = $pumpFactory->fromProductId($params['productId']);
        else
            throw new \InvalidArgumentException("FAIL");
    }
}

Is there a way to write this code better using PHP facilities? For example, I considered instead of $params to include $modelNumber and $productId directly, but I am not sure that it will be appropriate, since there will only be one legally valid parameter used at any one time.

Namely I need a way to create a pump using EITHER model number OR product id.

to call:

$factory = new AssemblyFactory();
$pump = $factory->makeAssembly(array(
    'productId' => 33
));
$pump = $factory->makeAssembly(array(
    'modelNumber' => 'AAA-50'
));

Answer 1

You could make your code "more dynamic" by creating a whitelist of "actionable keys" and the associated method name to be called, but that seems like unnecessary convolution.

Your current method is direct and literal and probably makes sense to those on your development team. I only have a few minor suggestions, but overall you should keep your basic structure.

It is not clear where $pump and $pumpFactory come from in your posted method, so I cannot offer advice on those variables. I also don't know what processes are actioned by the subsequent method calls, nor do I know the value type (boolean?, array?, string?, etc) or value of the return. A DocBlock would be a wise choice.

class AssemblyFactory
{
    /**
     * Generates assembly data based on model number or product id.
     *
     * @param array $params An array of pump details expected
     *                      to contain model number or product id.
     *
     * @throws InvalidArgumentException if $params does not contain a 
     *         model number or product id.
     *
     * @return string Pump assembly name 
     */
    public function makeAssembly(array $params)
    {
        if (array_key_exists('modelNumber', $params)) {
            return $pumpFactory->fromModelNumber($params['modelNumber']);
        }
        if (array_key_exists('productId', $params)) {
            return $pumpFactory->fromProductId($params['productId']);
        }
        throw new \InvalidArgumentException("FAIL");
    }
}

When seemingly simple code doesn't feel clear enough (or if you really love documentation blocks), then use DocBlocks to explain your script and avoid misinterpretation or lost time in the future.

PSR-5: PHPDoc

Here's an example that is relevant to your code: https://docs.phpdoc.org/references/phpdoc/tags/throws.html

Beyond your provided code, I feel that you should determine which lookup method is more efficient within your application (If this is a database lookup, is one column a Primary Key and another one not?). Make the more efficient process the default one.

Alternatively, if you have two separate collections of data containing redundant data, consider merging the data storage so that only one lookup is necessary.