2
\$\begingroup\$

Let's take the following example. Even if the number of parameters is acceptable, I find that the code block is too long.

Are these validations at the right place? Is there a better approach in such cases?

public function __construct(
    array $config,
    EntityManager $em,
    EventDispatcherInterface $dispatcher
) {
    $this->em = $em;
    $this->dispatcher = $dispatcher;
    if (isset($config['endPoint'])) {
        $this->endpoint = $config['endPoint'];
    } else {
        throw new ConfigurationException();
    }
    if (isset($config['partnerLogin']) && isset($config['partnerPass'])) {
        $this->partnerLogin = $config['partnerLogin'];
        $this->partnerPass = $config['partnerPass'];
    } else {
        throw new MissingCredentialsException();
    }
    if (isset($config['account_id'])) {
        $this->ids['account_id'] = $config['account_id'];
    } else {
        throw new MissingCredentialsException();
    }
    if (isset($config['user_id'])) {
        $this->ids['account_id'] = $config['account_id'];
    } else {
        throw new MissingCredentialsException();
    }
    $this->apiClient = $this->createApiClient();
}
\$\endgroup\$
1
  • \$\begingroup\$ I'm closing this question as off-topic because, as it is currently framed, it is asking about a best practice in general (performing validation in constructors), with the code presented merely as one example. It could be rephrased to be on-topic, by focusing on the task that this code actually accomplishes. See How to Ask. \$\endgroup\$ – 200_success Apr 13 '17 at 14:25
2
\$\begingroup\$

I would probably create a separate validateConfig() method on the class, such that your code might look like:

public function __construct(
    array $config,
    EntityManager $em,
    EventDispatcherInterface $dispatcher
) {
    $this->validateConfig($config);  
    $this->em = $em;
    $this->dispatcher = $dispatcher;
    $this->endpoint = $config['endPoint'];
    // set other config-driven properties here
    $this->apiClient = $this->createApiClient();
}

protected function validateConfig($config) {
    if (!isset($config['endPoint'])) {
        throw new ConfigurationException();
    }
    if (!isset(
        $config['partnerLogin'],
        $config['partnerPass'],
        $config['account_id'],
        $config['user_id']
    ) {
        throw new MissingCredentialsException();
    }
}

However, that might not be getting you where you ultimately would like to go. I would suggest that with a config with as many "properties" (your array keys) as you have, that you might consider creating an actual config class to be passed to constructor. This would allow you to type hint against the object type and place logic for validating the config into its own class. This class would then be 100% assured the dependencies it needs in its constructor are met.

This approach could easily be achieved without much change to the calling code. Instead of something like:

$object = new YourClass($config, $em, $dispatcher);

You could simply do:

$object = new YourClass(
    new YourClassConfig($array),
    $em,
    $dispatcher
);

Also, if you are going through the trouble of validating dependencies in this manner, is only looking for presence of an array value being set going far enough to make sure your object is set up in proper state? What if these values are set to empty strings or other non-sensical values for your context?

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.