current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

I have an array with the following structure

Array 
( 
    [0] => Array 
        ( 
            [area] => Area ABC 
            [activity] => Array 
                ( 
                    [0] => Activity 1
                    [1] => Activity 2
                ) 
        ) 
)

I'm trying to render this array in the following way

Area ABC

  1. Activity 1
  2. Activity 2

And am currently using the following code to achieve this

$status = 'closed';
foreach($suggestions as $suggestion)
    {
    if($currentarea != $suggestion['area'])
        {
        echo "<h3>";
        echo $suggestion['area'];
        echo "</h3>";
        if($status == 'closed')
            {
            echo "<ol>";
            $status = 'open';
            }
        }
    foreach($suggestion['activity'] as $activity)
        {
            echo "<li>";
            echo $activity;
            echo "</li>";
        }
    if($status == 'open')
        {
            echo "</ol>";
            $status = 'closed';
        }
    $currentarea = $suggestion['area'];
    }

Is this the best way of doing it, or is there a better way, I will have more areas and activities, but the structure stays the same.

share|improve this question
    
I guess you're intializing status before the first loop, am I right ? Could you put that in your code, just so there is no confusion possible ? –  Loufylouf Jul 3 at 19:22
    
yes, that is correct, updated. Sorry for the confusion –  TDsouza Jul 3 at 19:34
    
Your code seems more complicated than it needs to be for this simple example. Please elaborate on your example data to include more areas and activities. –  200_success♦ Jul 3 at 19:49

2 Answers 2

up vote 3 down vote accepted

This won't work if each suggestion does not start a new area. 

foreach($suggestions as $suggestion)
    {
    if($currentarea != $suggestion['area'])
        {
        echo "<h3>";
        echo $suggestion['area'];
        echo "</h3>";
        if($status == 'closed')
            {
            echo "<ol>";
            $status = 'open';
            }
        }
    foreach($suggestion['activity'] as $activity)
        {
            echo "<li>";
            echo $activity;
            echo "</li>";
        }
    if($status == 'open')
            {
            echo "</ol>";
            $status = 'closed';
            }
    $currentarea = $suggestion['area'];
    }

You close on every iteration that it's open and add list items on every iteration, but you only open if the area changes. 

foreach($suggestions as $suggestion)
{
    echo '<h3>';
    echo $suggestion['area'];
    echo "</h3>\n";
    echo "<ol>\n";

    foreach($suggestion['activity'] as $activity)
    {
        echo '  <li>';
        echo $activity;
        echo "</li>\n";
    }

    echo "</ol>\n";
}

This code is simpler and will work more reliably.

This is the wrong place to discuss how to write new code. But if you want to get the previous concept working, you'll want to open and close inside the if that checks if the area has changed. Or pull off the first element of the list so that you can treat it differently. Or make a new list where there's only one suggestion per area, combining multiple suggestions with the same area into one.

share|improve this answer
    
Each activity will always have an area attached to it, but yes, looking at your code I do feel I was over engineering a bit. I was working with different types of formats for the initial array, and in the process this code evolved (not to it's best in this case I guess). Thanks –  TDsouza Jul 3 at 20:15
up vote 3 down vote

I'll add to @mdfst13's answer that you should use concatenation when you want to display your strings. I get the idea that you want to separate things, but really, 3 lines to display a list item ?

One article I read gave a good example about that, with the ternary operator. Let's say you have to display some thing, and at a specific location, you could have the singular or plural form of a name (like "one knife" or "x knives"), depending on the value of a variable. Are you gonna really spend 6 lines of code (in the case you don't over-use newlines) just for that (1 for the first part of the sentence, 4 for the if/else, 1 to resume the sentence) ? Is that condition what is important in your code ?

Having a well-spaced code is important, but sometimes concision is also important. You just have to find the right balance.

share|improve this answer

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.