CODEWITHSUNDEEP
php
tommy505
tommy505

Reputation: 1

More elegant way to write these conditions?

out of curiosity I'm wondering if there's a more elegant way to write the conditionals below? I can't see a shorter way of writing it but it feels pretty clunky, so any suggestions welcome! 

        // Check whether this page has any visuals
        if (count($this->page->pagevisuals->find_all()) > 0)
        {
            // Ok to go ahead and assign
            $visual = $this->page->pagevisuals->find_all();
        }
        // If this is a sub page, parent page may have visuals we can use
        elseif (count($this->page->parent->pagevisuals->find_all()) > 0)
        {
             $visual = $this->page->parent->pagevisuals->find_all();
        }
        // If two levels deep, grandparent page might have visuals
        elseif (count($this->page->parent->parent->pagevisuals->find_all()) > 0)
        {
            $visual = $this->page->parent->parent->pagevisuals->find_all();
        }

Upvotes: 0

Views: 100

Answers (6)

Ariel
Ariel

Reputation: 26773

$visual = $this->page->pagevisuals->find_all()
or $visual = $this->page->parent->pagevisuals->find_all()
or $visual = $this->page->parent->parent->pagevisuals->find_all();

What do you do if none of them match? In this code it will be set to the last one, which is not the same as what you did. (In your code $visual was not touched if none matched, in this code it will be set to zero. You could add or $visual = -1 or something similar.)

You can make a loop if you want to avoid all the ->parent, but you'll need some terminator.

$el = $this->page;
while(!$visual = $el->pagevisuals->find_all()) {
  $el = $el->parent;
}

This could run forever if it never matches, but I don't know enough about your application to suggest a termination condition - you could add a counter, or something else.

Upvotes: 0

fdreger
fdreger

Reputation: 12505

You might want make two changes in your code:

  1. Ensure that getVisuals() returns an empty array instead of null in case there are no visuals
  2. Consider making a null-object - a singleton page instance that has no visuals and has itself as a parent. It might have a method like isNull() so you can easily test if a given page is the null page.

If you make the two adjustments, most of the code concerning visuals will become easier to write and debug.

Getting all the visuals for two levels (I assume you don't want recursion):

$visuals = array_merge(
    $this->page->pagevisuals->find_all(),
    $this->page->parent->pagevisuals->find_all(),
    $this->page->parent->parent->pagevisuals->find_all(),
);

Getting the visuals of the page OR of parent OR of grand parent:

   ($visuals = $this->page->pagevisuals->find_all()) ||
   ($visuals = $this->page->parent->pagevisuals->find_all()) ||
   ($visuals = $this->page->parent->parent->pagevisuals->find_all());

Recursive functions would be much simpler too (this is a method to add to the page object):

public function findRecursive(){
    $my_visuals = $this->pagevisuals->find_all()
    return $this->parent->isNull()?
               $my_visuals
               : array_merge($my_visuals, $this->parent->findRecursive());
}

Upvotes: 0

kjetilh
kjetilh

Reputation: 4976

You could make a recursive method to get rid of those nasty conditionals. Also you're calling the find_all() method twice for every conditional branch which doubles the process time.

Here's an attempt at a recursive function (might not work though, recursive functions are always a bit tricky!). Beware of infinite loops.

<?php
$visual = $this->page->find_all_visuals();

class Page {
    function find_all_visuals()
    {
        $found = $this->pagevisuals->find_all();

        if (count($found) > 0) {
            return $found;
        } else if ($this->parent == null) {
            return null;
        } else {
            return $this->parent->find_all_visuals();
        }
    }
}
?>

Upvotes: 0

Felix Kling
Felix Kling

Reputation: 816840

A recursive approach:

function getVisuals($root) {
    $visuals = $root->pagevisuals->find_all();
    if(count($visuals) === 0 && isset($root->parent)) {
        $visuals = getVisuals($root->parent);
    }
    return $visuals;
}

$visuals = getVisuals($this->page);

If you have control over whatever class $this->page is an instance of, then you can make it an instance method.

Upvotes: 0

Seldaek
Seldaek

Reputation: 42056

You can write a loop instead:

$page = $this->page;
$visual = null;
while (!$visual && $page) {
    $visual = $page->pagevisuals->find_all();
    $page = $page->parent;
}

I believe this is equivalent, and will work no matter how many levels of parents/nesting you have.

Upvotes: 1

pb149
pb149

Reputation: 2298

You could assign $this->page to a variable and begin the statements with that, for a very slight improvement.

Alternatively, you could create nested ternary statements to assign $visual, but that's certainly not recommended practice.

Upvotes: 0

Related Questions