how-to-refactor-this.php

<?php
/**
* Checks for a partial match in the geocode response
* 
* @return boolean
*/
public function isPartialResponse() {	
	if (isset($this->geocodeResponse['results'][0]['partial_match']) && $this->geocodeResponse['results'][0]['partial_match'] == 'true')
		return true;

	return false;
}

Not sure how to refactor this...

Code

public function isPartialResponse()
{   
    if (isset($this->geocodeResponse['results'][0]['partial_match']) && $this->geocodeResponse['results'][0]['partial_match'] === true)
        return true;

    return false;
}

I feel like it is too long due to repetition. Need to figure out how or if it should even be reduced more to begin with. The problem is with geocodeResponse['results'][0]['partial_match'] because that array location may not actually be set / returned from the Google Maps API unless it is true, so I need to check if it exists before I can run any further logic on it - this also means I cannot store the value in a separate variable to avoid repeating myself because if the key isn't set, then I will get an undefined index notice error from $partial_match = $this->geocodeResponse['results'][0]['partial_match'];

Alternatively, because I know the Google Maps API will only return the [partial_match] in the json response if it is actually true, then I can just go ahead and only do an isset and trust Google. However, I feel like for the sake of rigidity I should also check it myself again for the value of true rather than just accepting it at face value.

Here is what I've been trying to do for improved readability.

Option 1 - Separate logic structures

Still some repetition but is much clearer than consolidating the conditionals onto one line as in the initial code sample. Note, that if the conditions being checked were short enough (under 80 chars) then it would make sense to consolidate the conditionals.

The other thing this solution has going for it is that it has the potential to escape after it has checked just one condition rather than two conditions as in the original code. There is an argument to be had here - if we are programming for maintainability, then the ease of a human interpreting and read the code should take precedence over saving a few cpu cycles (sorry Skynet). However, it looks quite readable to me.

public function isPartialResponse()
{   
    if ( ! isset($this->geocodeResponse['results'][0]['partial_match']))
        return false;

    if ($this->geocodeResponse['results'][0]['partial_match'] !== 'true')
        return false;

    return true;
}

Option 2 - Move some logic to its own separate method

Here I check that the array key geocodeResponse['results'][0]['partial_match'] is set by using a separate method and then calling it in isPartialResponse().

However, now I have two issues I do not like:

• all I've done is created more code and verbosity, which may not necessarily be a bad thing if it improves clarity but am not convinced it is marginally any clearer
• still need to repeat geocodeResponse['results'][0]['partial_match'] twice

// new method
public function issetPartialResponse()
{
    if (isset($this->geocodeResponse['results'][0]['partial_match']))
        return true;

    return false
}

public function isPartialResponse()
{   
    if ($this->issetPartialResponse) && $this->geocodeResponse['results'][0]['partial_match'] == 'true')
            return true;

    return false;
}

Option 2a - Move logic to variable

Along the same thought, but store the isset value in a variable. Still, the problem remains ...

• still need to repeat geocodeResponse['results'][0]['partial_match'] twice

public function isPartialResponse()
{   
    $isset = isset($this->geocodeResponse['result'][0]['partial_match']);

    if ($isset && $this->geocodeResponse['results'][0]['partial_match'] == 'true')
        return true;

    return false;
}

Option 3 - Use the ? operator

Doesn't really help...

public function isPartialResponse()
{   
    $partial_match = isset($this->getGeocodeResponse['results'][0]['partial_match']) ? $this->getGeocodeResponse['results'][0]['partial_match'] : false;

    // or alternatively
    $partial_match = isset($this->getGeocodeResponse['results'][0]['partial_match']) 
                   ? $this->getGeocodeResponse['results'][0]['partial_match']
                   : false;                                                 

    return $partial_match;
}

Option 4 - Wait for Null Coalesce Operator (??) in PHP7

C and Perl already have this so can just wait to implement it

Decision / Solution

Initially I went with option 1, opting to use two independent if structures which also allowed for early escaping. The one problem I have with this is the alternating false, true, and default false return pathway.

/**
 * Checks if the geocode json response also returned a field for partial match. 
 * 
 * Need to refactor at one point, do not like the alternating false, true, false structure.
 * One solution is to nest the if statements.
 *
 * @return boolean
 */
public function isPartialResponse()
{   
    if ( ! isset($this->geocodeResponse['results'][0]['partial_match']))
        return false;

    if ($this->geocodeResponse['results'][0]['partial_match'] === true)
        return true;

    return true;
}

I could instead change the second if condition to say...

if ($this->geocodeResponse['results'][0]['partial_match'] !== true)
    return false;

instead of...

if ($this->geocodeResponse['results'][0]['partial_match'] === true)
    return true;

and then the return structure would be false, false, default to true. Also worth noting here is that PHP interprets [partial_match] = 1 as boolean true.

However...

I have decided to use the following code. It still allows for escaping early on the first conditional if and also reduces the return pathways to two, at the expense of having a nested if structure.

    /**
     * Checks if the geocode json response also returned a field for partial match.
     * 
     * @return boolean
     */
    public function isPartialResponse()
    {
        // avoid an undefined index notice error
        if (isset($this->geocodeResponse['results'][0]['partial_match']))

            // php interpets this json array key as a boolean, therefore the conditional must test for boolean type
            if ($this->geocodeResponse['results'][0]['partial_match'] === true)
                return true;

        return false;
    }