CODEWITHSUNDEEP
php
Shixons
Shixons

Reputation: 197

How to simplify if conditions?

How can I simplify if conditions, because per each condition I make a new if/elseif and its a lot of code, this is what I have:

$chapters = array('1:data1', '2:data2', '4:datax', '3:datag');
sort($chapters);
$screenshots = array('1:screen1', '2:screen2', '3:screen3', '4:go4');
$chapterCount = count($chapters);
$chapterItems = 0;

foreach ($screenshots as $key => $screenshot) {
    $screenshotInfo = explode(':', $screenshot);
    $screen[$screenshotInfo[0]] = $screenshotInfo[1];
}

foreach ($chapters as $chapter) {
    $chapterInfo = explode(':', $chapter);
    $chapterNumber = current($chapterInfo);
    // If is the first chapter
    if ($chapterNumber == 1) {
        echo '<li>'.$chapterInfo[0].'</li>';
        // If have only one chapter
        if ($chapterItems+1 == $chapterCount) {
        echo '<li>'.$screen[$chapterNumber].'</li>';
        }
    }
    // If is a new chapter
    elseif ($currentNumber != $chapterNumber) {
        echo '<li>'.$screen[$chapterNumber-1].'</li>';
        echo '<li>'.$chapterInfo[0].'</li>';
        // If is new and last chapter
        if ($chapterItems+1 == $chapterCount) {
        echo '<li>'.$screen[$chapterNumber].'</li>';
        }
    }
    // If is the last chapter
    elseif ($chapterItems+1 == $chapterCount) {
        echo '<li>'.$chapterInfo[0].'</li>';
        echo '<li>'.$screen[$chapterNumber].'</li>';
    }
    else {
        echo '<li>'.$chapterInfo[0].'</li>';
    }

    $currentNumber = $chapterNumber;
    $chapterItems++;
}

That code works perfect on my tests but I'm sure it have a lot of unneeded code.

Upvotes: 0

Views: 2538

Answers (3)

Guilherme Cunha Eloi Santos
Guilherme Cunha Eloi Santos

Reputation: 148

If you check the original code you will see that: 

echo '<li>'.$chapterInfo[0].'</li>';

will be printed in every scenario. So we don't actually need an if for that. The only checks you need is: isNew and not isFirst

if ($currentNumber != $chapterNumber && $chapterNumber != 1)

so we can print the 'new header': 

echo '<li>'.$screen[$chapterNumber-1].'</li>';

And if it's the last one, so we can print the last item footer: 

echo '<li>'.$screen[$chapterNumber].'</li>';

So we can simplify by having the bellow code:

foreach ($chapters as $chapter) {
    $chapterInfo = explode(':', $chapter);
    $chapterNumber = current($chapterInfo);
    // If is new AND is not the first print the 'New item header'
    if ($currentNumber != $chapterNumber && $chapterNumber != 1) {
        echo '<li>'.$screen[$chapterNumber-1].'</li>';
    }
    // This was being printed in every scenario, so we don't need a if for that
    echo '<li>'.$chapterInfo[0].'</li>';
    // If it is the last, print the 'Last item footer'
    if ($chapterItems+1 == $chapterCount) {
        echo '<li>'.$screen[$chapterNumber].'</li>';
    }

    $currentNumber = $chapterNumber;
    $chapterItems++;
}

if your goal is just to remove if/else and get easier code coverage you can do the same thing but only with ternaries like that:

foreach ($chapters as $chapter) {
    $chapterInfo = explode(':', $chapter);
    $chapterNumber = current($chapterInfo);

    echo ($currentNumber != $chapterNumber && $chapterNumber != 1) ? sprintf('<li> %s </li>', $screen[$chapterNumber - 1]) : '';
    echo sprintf('<li> %s </li>', $chapterInfo[0]);
    echo ($chapterItems + 1 == $chapterCount)? sprintf('<li> %s </li>', $screen[$chapterNumber]): '';

    $currentNumber = $chapterNumber;
    $chapterItems++;
}

Upvotes: 0

Sabari
Sabari

Reputation: 6335

May be you could use a function for all the if-conditions. Since all the if-conditions inside are same we could use something like:

$chapters = array('1:data1', '2:data2', '4:datax', '3:datag');
sort($chapters);
$screenshots = array('1:screen1', '2:screen2', '3:screen3', '4:go4');
$chapterCount = count($chapters);
$chapterItems = 0;

foreach ($screenshots as $key => $screenshot) {
    $screenshotInfo = explode(':', $screenshot);
    $screen[$screenshotInfo[0]] = $screenshotInfo[1];
}

foreach ($chapters as $chapter) {
    $chapterInfo = explode(':', $chapter);
    $chapterNumber = current($chapterInfo);
    if ($chapterNumber == 1) { // If is the first chapter
        echo '<li>'.$chapterInfo[0].'</li>';
        echo compare($chapterItems,$chapterCount,$screen,$chapterNumber); // If have only one chapter
    } elseif ($currentNumber != $chapterNumber) { // If is a new chapter
        echo '<li>'.$screen[$chapterNumber-1].'</li>';
        echo '<li>'.$chapterInfo[0].'</li>';
        echo compare($chapterItems,$chapterCount,$screen,$chapterNumber); // If is new and last chapter
    } elseif ($chapterItems+1 == $chapterCount) { // If is the last chapter
        echo '<li>'.$chapterInfo[0].'</li>';
        echo '<li>'.$screen[$chapterNumber].'</li>';
    } else {
        echo '<li>'.$chapterInfo[0].'</li>';
    }

    $currentNumber = $chapterNumber;
    $chapterItems++;
}

function compare($chapterItems,$chapterCount,$screen,$chapterNumber) {

    if ($chapterItems+1 == $chapterCount) {
       return '<li>'.$screen[$chapterNumber].'</li>';
    } 

    return false;
}

Hope this helps you :)

Upvotes: 1

Chris Sobolewski
Chris Sobolewski

Reputation: 12925

If brevity is of more interest than readability, you can use ternary statements.

$foo = true;
$bar = $foo ? 'something' : 'nothing';
echo $bar;
 //returns 'something'
$foo = false;
$bar = $foo ? 'something' : 'nothing';
$echo bar;
 //returns 'nothing'

Upvotes: 1

Related Questions