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.

\$\endgroup\$

1 Answer 1

Reset to default
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;
\$\endgroup\$
4
  • 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\$ Commented Feb 28, 2014 at 13:51
  • \$\begingroup\$ @PetrePătraşc: Thank you, I've fixed that. \$\endgroup\$ Commented Feb 28, 2014 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\$ Commented Feb 28, 2014 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\$ Commented Feb 28, 2014 at 14:29

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.