2
\$\begingroup\$

I have a data structure that allows for inheritance of properties, in such a way that if a property is not defined within a context, the second most specific context is checked until said property is found. Simplified it's basically the following:

function getCarPartColor($carData, $owner, $brand, $part) {
    $color = false;

    if(isset($carData->{$owner}->{$brand}->{$part})) {
        $color = $carData->{$owner}->{$brand}->{$part};

    } else if(isset($carData->{$owner}->{$brand}->allparts)) {
        $color = $carData->{$owner}->{$brand}->allparts;

    } else if(isset($carData->{$owner}->allbrands->allparts)) {
        $color = $carData->{$owner}->allbrands->allparts;

    } else if(isset($carData->allowners->allbrands->allparts)) {
        $color = $carData->allowners->allbrands->allparts;
    }

    return $color;
}

Where $carData is the data structure and the rest are the querying variables. If I were to call:

getCarPartColor($carData, "AnthonyNewel", "opel", "engine");

But I hadn't defined any color for $carData->AnthonyNewel->opel->engine, then it checks if a color is set on $caraData->AnthonyNewel->opel->allparts, and fetches this one instead if it exists. If it's not there, it then checks if it's defined for allbrands, and it goes on.

While the code above works, one can't help but notice the repetition. Is there a less verbose way to write this while still being maintanable (i.e. not a long ternary operator)?

\$\endgroup\$
  • \$\begingroup\$ What are the expected returns of each scenario? \$\endgroup\$ – Brett Santore Jul 28 '14 at 17:41
  • \$\begingroup\$ @BrettSantore the same kind of value, only within a different "context" of the data. Think of it as the following: somebody asks what color is the engine of AnthonyNewel's Opel car. The function replies well I don't know the color of specifically the engine, but I know that all AnthonyNewel's Opel car engines are blue, so take blue. OR Well I don't know about his opel cars specifically but I know that all engines of all the cars AnthonyNewel owns are blue, so take blue. It's a little bit like class inheritance, but for data. \$\endgroup\$ – Mahn Jul 28 '14 at 19:57
  • \$\begingroup\$ So imagine the string "blue" as the return value in all of the possible scenarios (for the sake of simplicity), only in some cases it will be defined in more specific data paths than others (depending on the arguments passed) \$\endgroup\$ – Mahn Jul 28 '14 at 20:02
  • \$\begingroup\$ @EliasVanOotegem No it will not. PHP does not generate notices, warnings or errors on unset properties when isset is used (that's the whole point of using isset first). With the code above there can never be an scenario where PHP throws notices, warning or errors because every single path is tested with isset before any usage. \$\endgroup\$ – Mahn Jul 29 '14 at 8:51
  • \$\begingroup\$ I'm sorry, i guess more specifically, what is returned when the function fails to find a string? \$\endgroup\$ – Brett Santore Jul 29 '14 at 12:44
3
\$\begingroup\$

Expanding on k_smd's suggestion of using a loop, you could easily make this function a tad more generic using an array as a second argument.
I'd also strongly recommend you use type-hints, and doc-blocks to make your code more maintainable. And as ever looking at the coding standards never hurts.

Anyway, here's a slightly more concise version of your function:

/**
 * @param \stdClass $car
 * @param array $properties
 * @return mixed|bool
 */
function getCarPartColor(stdClass $car, array $properties)
{
    $defaults = array('allowners', 'allbrands', 'allparts');
    //ensure numeric indexes:
    $properties = array_values($properties);
    $current = $car;//current object to evaluate
    for ($i=0, $j=count($properties);$i<$j;++$i)
    {
        if (isset($current->{$properties[$i]}))//given property exists
            $current = $current->{$properties[$i]};
        elseif (isset($current->{$defaults[$i]}))//default exists
            $current = $current->{$defaults[$i]};
        else//none exist, return false
            return false;
    }
    return $current;
}

With minimal effort, you can make this function even more generic (and re-usable). It would suffice to pass the defaults as third argument, and you can use this function as a generic pseudo-recursive getter:

/**
 * @param \stdClass $car
 * @param array $properties
 * @param array $defaults
 * @return mixed|bool
 * @throws \InvalidArgumentException
 */
function getSubProperty(stdClass $obj, array $properties, array $defaults)
{
    //make sure the size of both arrays match
    $j = count($properties);
    if (!$j || $j > count($defaults))
        throw new \InvalidArgumentException(
            sprintf(
                'Default argument should contain at least %d values, instead saw %d',
                $j,
                count($defaults)
            )
        );
    //call array_values on array arguments, to ensure numeric indexes
    $current = $obj;
    for ($i=0, $j=0;$i<$j;++$i)
    {
        if (isset($current->{$properties[$i]}))//given property exists
            $current = $current->{$properties[$i]};
        elseif (isset($current->{$defaults[$i]}))//default exists
            $current = $current->{$defaults[$i]};
        else//none exist, return false
            return false;
    }
    return $current;
}

Of course, you might have spotted a "feature" you might not care for too much. Suppose you pass an object like this:

$foo = (object) array(
    'foobar' => (object) array(
         'property' => (object) array(
             'value' => 123
         )
    )
);//$foo->foobar->propery->value

You could then call this last function, trying to get $foo->something->property->value and pass array('foobar', 'foobar', 'foobar') as the $defaults array. The result would be that, during the first iteration of the for loop, the $properties[$i] value (something) would not be found, and instead, the $defaults value (foobar) will be used. The next iterations would then pan out just fine, and in the end, the function will return $foo->foobar->property->value.
This, then, is a potential source of mysterious, and difficult to trace bugs. But don't worry. It's a fairly easy thing to fix:

/**
 * @param \stdClass $car
 * @param array $properties
 * @param array $defaults
 * @return mixed|bool
 * @throws \InvalidArgumentException
 */
function getSubProperty(stdClass $obj, array $properties, array $defaults)
{
    //same as above
    for ($i=0, $j=0;$i<$j;++$i)
    {
        if (!isset($current->{$properties[$i]}))
            break;//break the loop!
        $current = $current->{$properties[$i]};
    }
    for ($i;$i<$j;++$i)
    {//only use defaults from here on end
        if (!isset($current->{$defaults[$i]}))
            return false;
        $current = $current->{$defaults[$i]};
    }
    return $current;
}

Putting it all together:
Finally, it might be useful to have the ability to switch back and forth between the $properties and $defaults provided in some cases. So in an ideal world, you might want to add another argument to your function, that allows the caller to decide what the appropriate course of action is, using something like this:

/**
 * @param \stdClass $car
 * @param array $properties
 * @param array $defaults
 * @param bool $switch = false
 * @return mixed|bool
 * @throws \InvalidArgumentException
 */
function getSubProperty(stdClass $obj, array $properties, array $defaults, $switch = false)
{
    $j = count($properties);
    if (!$j || $j > count($defaults))
        throw new \InvalidArgumentException(
            sprintf(
                'Default argument should contain at least %d values, instead saw %d',
                $j,
                count($defaults)
            )
        );
    $current = $obj;
    if ($switch === true)
    {//switch is true, function can switch back and forth
        for ($i=0, $j=0;$i<$j;++$i)
        {
            if (isset($current->{$properties[$i]}))//given property exists
                $current = $current->{$properties[$i]};
            elseif (isset($current->{$defaults[$i]}))//default exists
                $current = $current->{$defaults[$i]};
            else//none exist, return false
                return false;
        }
        return $current;
    }
    //No switching: if the $properties value can't be found, try defaults, or fail
    for ($i=0, $j=0;$i<$j;++$i)
    {
        if (!isset($current->{$properties[$i]}))
            break;//break the loop!
        $current = $current->{$properties[$i]};
    }
    for ($i;$i<$j;++$i)
    {
        if (!isset($current->{$defaults[$i]}))
            return false;
        $current = $current->{$defaults[$i]};
    }
    return $current;
}
\$\endgroup\$
  • \$\begingroup\$ I'm going to mark it as the accepted answer because I think this is the best alternative that the language would allow, though I think I will stick with the original since for the time being I find more maintainable (in that a new developer can grab the code and get what's going on fairly quick). But good thinking nonetheless! \$\endgroup\$ – Mahn Jul 30 '14 at 11:11
5
\$\begingroup\$

Long time reader, first time poster, welcoming of critique.

You could create an array populated with each variant of the property you're looking for:

    $is_set = array (
        $carData->{$owner}->{$brand}->{$part},
        $carData->{$owner}->{$brand}->allparts,
        $carData->{$owner}->allbrands->allparts,
        $carData->allowners->allbrands->allparts
    );

And then check each value in the array using a foreach loop which ends when the condition isset evaluates to true. The first property to evaluate to true is then returned which in turn breaks you out of the function (credit to Brett for the insight).

    foreach ($is_set as $this_set) {
        if isset($this_set) {
            return $this_set;
        }
    }

Putting it all together would look like:

function getCarPartColor($carData, $owner, $brand, $part) {

    $is_set = array (
        $carData->{$owner}->{$brand}->{$part},
        $carData->{$owner}->{$brand}->allparts,
        $carData->{$owner}->allbrands->allparts,
        $carData->allowners->allbrands->allparts
    );

    foreach ($is_set as $this_set) {
        if isset($this_set) {
            return $this_set;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I would wrap this in the original function getCarPartColor($carData, $owner, $brand, $part), you would than remove the break since the return would exit the function. \$\endgroup\$ – Brett Santore Jul 28 '14 at 17:43
  • 1
    \$\begingroup\$ could you explain this a little more please? (edit it into the post) \$\endgroup\$ – Malachi Jul 28 '14 at 17:49
  • \$\begingroup\$ Neat, but the problem here is that PHP would throw a warning if for instance $carData->{$owner}->{$brand}->{$part} isn't set at all, and the data structure I'm working with is built under the assumption that not setting a property at all is okay to inherit the property of the parent context. \$\endgroup\$ – Mahn Jul 28 '14 at 20:02

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.