Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
add comment

1 Answer

up vote 4 down vote accepted

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;
share|improve this answer
1  
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]; –  Petre Pătraşc Feb 28 at 13:51
    
@PetrePătraşc: Thank you, I've fixed that. –  palacsint Feb 28 at 14:05
    
I'll try running some code analysis on that and checking if the lack of ternary does bring it down. –  Petre Pătraşc Feb 28 at 14:07
    
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. –  Petre Pătraşc Feb 28 at 14:29
    
@PetrePătraşc: Good to hear that, thanks :) –  palacsint Feb 28 at 14:32
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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