1
\$\begingroup\$

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'
));
\$\endgroup\$
  • \$\begingroup\$ Does the params array possibly have more than one param? Semantics says yes, but your sample inputs say no. Might a params array have both identifiers? \$\endgroup\$ – mickmackusa Jul 11 at 3:01
  • \$\begingroup\$ Can you tell me how you hope to modify your code? Maybe I am misunderstanding the goal. Is this going the wrong direction? 3v4l.org/C6h1o (IMOyou aren't doing anything "bad" with your posted code.) Do we need to see more of your code? $pumpFactory seems to fly in from nowhere. \$\endgroup\$ – mickmackusa Jul 11 at 4:28
  • \$\begingroup\$ yes, $params can have more than one param. It can have other keys and values. It may even have both productId and modelNumber (doesn't matter which one takes precedence, although if required productId can take precedence over modelNumber). I was hoping for a way to better use PHP facilities, to where any errors with input or any other errors are shown early, so that I can fix them. Or there is more clarity in the code. With the array, it is not immediately clear what are the actual expected parameters, so future programmers will have harder time deciphering and understanding code. \$\endgroup\$ – Dennis Jul 11 at 15:59
  • \$\begingroup\$ I don't really see anything to improve. I find the code easy to read and straight forward. \$\endgroup\$ – mickmackusa Jul 11 at 21:38
  • \$\begingroup\$ Thank you. My concern was that $params was hiding the parameters. i.e. not clear how many parameters there are, or if valid parameters are being passed. \$\endgroup\$ – Dennis Jul 12 at 13:23
2
\$\begingroup\$

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.

\$\endgroup\$

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.