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 have the following code, which works as I need it to, however, it repeats itself over and over.

I'm relatively new to PHP and haven't grasped recursive functions yet. Which I've been told I'll need to understand as this function could potentially have many levels.

if ($_POST['parent'] == "None") {
        $parent = "";
    } else {
        $parent = $connection->real_escape_string($_POST['parent']);
    }

    if (!empty($parent)) {

        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        

        while($row = $query->fetch_array()) {

            $path_arr = explode('/',$row['path_to']);
            end($path_arr);
            $key = key($path_arr)-1;

            $parent1 = $path_arr[$key];

            if (!empty($parent1)) {

                $query = $connection->query("SELECT * FROM pages WHERE filename='$parent1'");

                while($row = $query->fetch_array()) {

                    $path_arr = explode('/',$row['path_to']);
                    end($path_arr);
                    $key = key($path_arr)-1;

                    $parent2 = $path_arr[$key];

                    if (!empty($parent2)) {

                        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent2'");

                        while($row = $query->fetch_array()) {

                            $path_arr = explode('/',$row['path_to']);
                            end($path_arr);
                            $key = key($path_arr)-1;

                            $parent3 = $path_arr[$key];

                            if (!empty($parent3)) {

                                $query = $connection->query("SELECT * FROM pages WHERE filename='$parent3'");

                                while($row = $query->fetch_array()) {

                                    $path_arr = explode('/',$row['path_to']);
                                    end($path_arr);
                                    $key = key($path_arr)-1;

                                    $parent4 = $path_arr[$key];

                                }
                            }
                        }
                    }
                }
            }
        }       
    }

    $path_to = rtrim(ltrim($parent4.'/'.$parent3.'/'.$parent2.'/'.$parent1.'/'.$parent.'/'.$filename,'/'),'/');

As you can see there are minimal differences within the nested while loops, but I'm really not sure how to make the function loop for each level.

I was pointed to this, and while I understand what the factorial is, I still am unsure on how to refactor my code into a looping function.

Any pointers?

UPDATE

Certainly moving in the right direction of what I'm hoping to achieve with Simon André Forsberg's suggestion

However (I probably should have made this clearer in my explanation), The loop(s) determine whether a 'file' (a database entry, masquerading as a file) has a parent 'file', returns that parent 'file', then checks that parent 'file' for a parent 'file' and returns it and so on and so on until there are no more parents.

It is used to create a dynamic path directory.

like so /parent4/parent3/parent2/parent1/filename/

The line of code that follows the loops (again, I probably should have included this), is below...

$path_to = rtrim(ltrim($parent4.'/'.$parent3.'/'.$parent2.'/'.$parent1.'/'.$parent.'/'.$filename,'/'),'/');

So each level of the loop outputs to this string.

I have a feeling I may have to do this using palacsint's method of taking the working part of each loop and creating that as a function.

I may be wrong but could Simon's method output to an array? first parent in pos 0, second in pos 1, etc?

However, then the query would have to have the filename='$parent' updated to look at the last array entry.

$query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");

Have I bitten off more than I can chew?

share|improve this question

2 Answers 2

up vote 7 down vote accepted

You seem to always end with the check: Is there another parent? Or as it can be formulated, while there is another parent, grab that parent.

Luckily for you, there are while loops.

while (!empty($parent)) {
    $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        
    while($row = $query->fetch_array()) {
        $path_arr = explode('/',$row['path_to']);
        end($path_arr);
        $key = key($path_arr)-1;
        $parent = $path_arr[$key];
        break; // break from the inner loop that fetches each row
    }
}

This code will continue looping while there is a parent available. Note that the new parent uses the same variable as the previous one, this is an important part in making this loop work.

Other suggestions:

  • Use prepared statements! It is good that you seem to sanitize your inputs by using the real_escape_string method. But prepared statements is always better. And then you wouldn't have to use the real_escape_string call.

  • Since this code only gets the first result, you don't need the inner while loop and can replace it by a if instead.

    while (!empty($parent)) {
        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        
        if ($row = $query->fetch_array()) {
            $path_arr = explode('/',$row['path_to']);
            end($path_arr);
            $key = key($path_arr)-1;
            $parent = $path_arr[$key];
        }
    }
    
  • I do have to ask though: What's the point in just grabbing the parent until there are no more parents? To store all the parents in an array, let's do like this:

    $parents = array();
    while (!empty($parent)) {
        $parents[] = $parent; // add parent to the array
        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        
        if ($row = $query->fetch_array()) {
            $path_arr = explode('/',$row['path_to']);
            end($path_arr);
            $key = key($path_arr)-1;
            $parent = $path_arr[$key];
        }
    }
    

Now, when this loop finishes, $parents contains all the parents that was not empty. Now you can loop through that using a foreach loop, or use it with implode or whatever you'd like :)

share|improve this answer
    
Thanks for this Simon, it's certainly moving toward what I'm trying to achieve. I've updated a couple of points on the question to try to explain better what I am trying to achieve. Basically, the code isn't just trying to find the last parent, it's outputting each parent that it finds, checking that parent for a parent and so on... –  Dijon Mar 19 at 9:09
    
@Dijon Updated last part of my answer to use an array. –  Simon André Forsberg Mar 19 at 11:04

If it's a function in itself, use guard clauses to make it flatten. If it's not, extract it out to separate function and use guard clauses in that. You can invert the !empty conditions and return or continue if it's true. With that you can reduce the indentation levels from nine to six.

if ($_POST['parent'] == "None") {
    $parent = "";
} else {
    $parent = $connection->real_escape_string($_POST['parent']);
}

if (empty($parent)) {
    return;
}

$query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        

while($row = $query->fetch_array()) {
    $path_arr = explode('/',$row['path_to']);
    end($path_arr);
    $key = key($path_arr)-1;

    $parent1 = $path_arr[$key];

    if (empty($parent1)) {
        continue;
    }
    ...
}

As the second step, you could create a function for

$path_arr = explode('/', $row['path_to']);
end($path_arr);
$key = key($path_arr)-1;
$parent1 = $path_arr[$key];

For example:

function get_parent_folder($filename) {
    $path_arr = explode('/', $filename);
    end($path_arr);
    $key = key($path_arr)-1;

    $parent1 = $path_arr[$key];
    return $parent1;
}

I guess instead of the array manipulation you could use PHP's built-in filename functions here which are probably well-tested and handle corner cases better than reinvented code:

function get_parent_folder($filename) {
    $dirname = dirname($filename);
    $parent_basename = basename($dirname);
    return $parent_basename;
}

(See also: Effective Java, 2nd edition, Item 47: Know and use the libraries It's a Java book and the author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

Usage:

$query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");        
while($row = $query->fetch_array()) {
    $parent1 = get_parent_folder($row['path_to']);

    if (empty($parent1)) {
        continue;
    }

    $query = $connection->query("SELECT * FROM pages WHERE filename='$parent1'");
    while($row = $query->fetch_array()) {
        $parent2 = get_parent_folder($row['path_to']);

        if (empty($parent2)) {
            continue;
        }

        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent2'");
        while($row = $query->fetch_array()) {
            $parent3 = get_parent_folder($row['path_to']);

            if (empty($parent3)) {
                continue;
            }

            $query = $connection->query("SELECT * FROM pages WHERE filename='$parent3'");

            while($row = $query->fetch_array()) {
                $parent4 = get_parent_folder($row['path_to']);

            }
        }
    }
}

After that it's easier to see the pattern and comes Simon's solution.

share|improve this answer
    
Thanks @palacsint, I'm thinking this may be the way I have to do it. I've updated the question to better explain the scenario, have a look and see what you think. I might be wrong but I can't use PHP's built in filename functions as the files are not real files, but database entries, masquerading as files. –  Dijon Mar 19 at 9:12
1  
@Dijon: Not all of the filename functions require real files. IIRC I've tried the code above and it worked without real files. –  palacsint Mar 19 at 9:17
1  
Great, I'll read the docs and implement it. –  Dijon Mar 19 at 9:22

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.