5
\$\begingroup\$

I'm trying to reduce the complexity of some methods, and I'm not exactly sure what approach to take. I'm currently building a PHP wrapper for a REST API, and the main problematic class is here:

https://github.com/petrepatrasc/blizzard-starcraft-api/blob/master/Service/ApiService.php

In short, since data retrieved from the API is not always consistent, I have to do this:

$portrait = new Portrait();
$portrait->setXCoordinate(isset($apiData['portrait']['x']) ? $apiData['portrait']['x'] : null)
    ->setYCoordinate(isset($apiData['portrait']['y']) ? $apiData['portrait']['y'] : null)
    ->setWidth(isset($apiData['portrait']['w']) ? $apiData['portrait']['w'] : null)
    ->setHeight(isset($apiData['portrait']['h']) ? $apiData['portrait']['h'] : null)
    ->setOffset(isset($apiData['portrait']['offset']) ? $apiData['portrait']['offset'] : null)
    ->setUrl(isset($apiData['portrait']['url']) ? $apiData['portrait']['url'] : null);
return $portrait;

What would be a better way of handling the problem, so that the method complexity goes down? I suppose that I can try and catch an "undefined index exception", but I kind of wanted to see if there's something a bit more elegant that I can use.

Improve this question
\$\endgroup\$
4
\$\begingroup\$

You could remove some duplication with a getPortraitValue function:

function getPortraitValue($apiData, $key) {
    $portrait = $apiData['portrait'];
    if (isset($portrait[$key])) {
        return $portrait[$key];
    }
    return null;
}

$portrait = new Portrait();
$portrait->setXCoordinate(getPortraitValue($apiData, 'x')
    ->setYCoordinate(getPortraitValue($apiData, 'y'))
    ->setWidth(getPortraitValue($apiData, 'w'))
    ->setHeight(getPortraitValue($apiData, 'h'))
    ->setOffset(getPortraitValue($apiData, 'offset')
    ->setUrl(getPortraitValue($apiData, 'url');
return $portrait;
Improve this answer
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Yep, was thinking the same thing at one point, but this solution is essentially just a wrapper for the inline if. I say this because it is in fact a tad incorrect - if $apiData['portrait'][$key] does not exist, you'll still get a "Undefined index" exception, so instead you have to do: if (isset($apiData['portrait'][$key])) return $apiData['portrait'][$key]; \$\endgroup\$ – Petre Pătraşc Feb 28 '14 at 13:51
  • \$\begingroup\$ @PetrePătraşc: Thank you, I've fixed that. \$\endgroup\$ – palacsint Feb 28 '14 at 14:05
  • \$\begingroup\$ I'll try running some code analysis on that and checking if the lack of ternary does bring it down. \$\endgroup\$ – Petre Pătraşc Feb 28 '14 at 14:07
  • \$\begingroup\$ Yep, you were right! Method has been improved! scrutinizer-ci.com/g/petrepatrasc/blizzard-starcraft-api/… Accepting your answer, as it is the right solution. \$\endgroup\$ – Petre Pătraşc Feb 28 '14 at 14:29
  • \$\begingroup\$ @PetrePătraşc: Good to hear that, thanks :) \$\endgroup\$ – palacsint Feb 28 '14 at 14:32

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.