0

i currently have 2 switch cases that basically use the same data, but has different returns (one switch case returns an image url, while the other returns a number), I don't think this is very good practice (Really Trying to code efficiently), Is there any chance of combining the two switch cases into one switch case?

Here they are:

function getImage($image,$image_url,$count){
    switch ($image) {
        case 1:
            if($count == 1){
                return "images/menu_slider/960x200/".basename($image_url);
            }else if($count == 2){
                return "images/menu_slider/595x200/".basename($image_url);
            }else if($count == 6){
                return "images/menu_slider/480x200/".basename($image_url);
            }else{
                return "images/menu_slider/470x200/".basename($image_url);
            }
        break;
        case 2:
            if($count == 2){
                return "images/menu_slider/365x200/".basename($image_url);
            }else if($count == 3 || $count == 4  ||$count == 5  ||$count == 6   ){
                return "images/menu_slider/250x200/".basename($image_url);
            }else if ($count == 7){
                return "images/menu_slider/250x300/".basename($image_url);
            }else{
                return "images/menu_slider/250x400/".basename($image_url);
            }
        break;
        case 3:
            if($count == 3){
                return "images/menu_slider/240x200/".basename($image_url);
            }else{
                return "images/menu_slider/240x100/".basename($image_url);
            }
        break;
        case 4:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 5:
            if($count == 5){
                return "images/menu_slider/960x100/".basename($image_url);
            }else if($count == 6){
                return "images/menu_slider/480x100/".basename($image_url);
            }else if($count == 7){
                return "images/menu_slider/235x100/".basename($image_url);
            }else if($count == 8){
                return "images/menu_slider/235x200/".basename($image_url);
            }else{
                return "images/menu_slider/235x141/".basename($image_url);
            }
        break;
        case 6:
            if($count == 6){
                return "images/menu_slider/480x100/".basename($image_url);
            }else if($count == 7){
                return "images/menu_slider/235x100/".basename($image_url);
            }else if($count == 8){
                return "images/menu_slider/235x200/".basename($image_url);
            }else{
                return "images/menu_slider/235x141/".basename($image_url);
            }
        break;
        case 7:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 8:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 9:
            return "images/menu_slider/470x60/".basename($image_url);
        break;
    }
}

function getLinks($links,$count){
    switch ($links) {
        case 1:
            if($count == 1){
                //return "12 Links 4 Cols";
                return 12;
            }else{
                //return "12 Links 2 Cols";
                return 12;
            }
        break;

        case 2:
            if($count == 6){
                //return "6 Links";
                return 6;
            }else if($count == 7){
                //return "12 or 14 Links";
                return 12;
            }else{
                //return "12 Links";
                return 12;
            }
        break;

        case 3:
            if($count == 3){
                //return "6 Links";
                return 6;
            }else{
                //return "3 Links";
                return 3;
            }
        break;

        case 4:
            //return "3 Links";
            return 3;
        break;

        case 5:
            if($count == 5){
                //return "12 Links 4 col";
                return 12;
            }else if($count == 6){
                //return "6 Links 2 cols";
                return 6;
            }else if($count == 7){
                //return "3 Links";
                return 3;
            }else if($count == 8){
                //return "6 or 8 Links";
                return 6;
            }else{
                //return "4 Links";
                return 4;
            }
        break;

        case 6:
            if($count == 6){
                //return "6 Links 2 cols";
                return 6;
            }else if($count == 7){
                //return "3 Links";
                return 3;
            }else if($count == 8){
                //return "6 or 8 Links";
                return 8;
            }else{
                //return "4 Links";
                return 4;
            }
        break;

        case 7:
            //return "3 Links";
            return 3;
        break;

        case 8:
            //return "3 Links";
            return 3;
        break;

        case 9:
            //return "2 Links";
            return 2;
        break;
    }
}

and they are called like so:

$linkCount = getLinks($counter,$count);
getImage($image,$image_url,$count);

I don't know if im being unnecessary, But it just doesn't seem logical to me...

Any Help Greatly Appreciated.

Here's the entire file if required:

<?php
$image_url = "";
//Conditional Images
function getLinks($links,$count){
    switch ($links) {
        case 1:
            if($count == 1){
                //return "12 Links 4 Cols";
                return 12;
            }else{
                //return "12 Links 2 Cols";
                return 12;
            }
        break;

        case 2:
            if($count == 6){
                //return "6 Links";
                return 6;
            }else if($count == 7){
                //return "12 or 14 Links";
                return 12;
            }else{
                //return "12 Links";
                return 12;
            }
        break;

        case 3:
            if($count == 3){
                //return "6 Links";
                return 6;
            }else{
                //return "3 Links";
                return 3;
            }
        break;

        case 4:
            //return "3 Links";
            return 3;
        break;

        case 5:
            if($count == 5){
                //return "12 Links 4 col";
                return 12;
            }else if($count == 6){
                //return "6 Links 2 cols";
                return 6;
            }else if($count == 7){
                //return "3 Links";
                return 3;
            }else if($count == 8){
                //return "6 or 8 Links";
                return 6;
            }else{
                //return "4 Links";
                return 4;
            }
        break;

        case 6:
            if($count == 6){
                //return "6 Links 2 cols";
                return 6;
            }else if($count == 7){
                //return "3 Links";
                return 3;
            }else if($count == 8){
                //return "6 or 8 Links";
                return 8;
            }else{
                //return "4 Links";
                return 4;
            }
        break;

        case 7:
            //return "3 Links";
            return 3;
        break;

        case 8:
            //return "3 Links";
            return 3;
        break;

        case 9:
            //return "2 Links";
            return 2;
        break;
    }
}
function getImage($image,$image_url,$count){
    switch ($image) {
        case 1:
            if($count == 1){
                return "images/menu_slider/960x200/".basename($image_url);
            }else if($count == 2){
                return "images/menu_slider/595x200/".basename($image_url);
            }else if($count == 6){
                return "images/menu_slider/480x200/".basename($image_url);
            }else{
                return "images/menu_slider/470x200/".basename($image_url);
            }
        break;
        case 2:
            if($count == 2){
                return "images/menu_slider/365x200/".basename($image_url);
            }else if($count == 3 || $count == 4  ||$count == 5  ||$count == 6   ){
                return "images/menu_slider/250x200/".basename($image_url);
            }else if ($count == 7){
                return "images/menu_slider/250x300/".basename($image_url);
            }else{
                return "images/menu_slider/250x400/".basename($image_url);
            }
        break;
        case 3:
            if($count == 3){
                return "images/menu_slider/240x200/".basename($image_url);
            }else{
                return "images/menu_slider/240x100/".basename($image_url);
            }
        break;
        case 4:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 5:
            if($count == 5){
                return "images/menu_slider/960x100/".basename($image_url);
            }else if($count == 6){
                return "images/menu_slider/480x100/".basename($image_url);
            }else if($count == 7){
                return "images/menu_slider/235x100/".basename($image_url);
            }else if($count == 8){
                return "images/menu_slider/235x200/".basename($image_url);
            }else{
                return "images/menu_slider/235x141/".basename($image_url);
            }
        break;
        case 6:
            if($count == 6){
                return "images/menu_slider/480x100/".basename($image_url);
            }else if($count == 7){
                return "images/menu_slider/235x100/".basename($image_url);
            }else if($count == 8){
                return "images/menu_slider/235x200/".basename($image_url);
            }else{
                return "images/menu_slider/235x141/".basename($image_url);
            }
        break;
        case 7:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 8:
            return "images/menu_slider/240x100/".basename($image_url);
        break;
        case 9:
            return "images/menu_slider/470x60/".basename($image_url);
        break;
    }
}
?>
<?php
    $document = JFactory::getDocument();
    $style= array();
?>
<?php foreach ($menu as $item): ?>
<?php 
    if($item->theme_title){
        $class = $item->theme_title;

        $style[] = '.'.$class.' > .cover {
            color:'.$item->foreground_text_color.';
            overflow: visible !important;
        }';

        $style[] = '.'.$class.' > .slide-background {
            background:'.$item->background_color.';
            color:'.$item->background_text_color.';
        }';

        $style[] = '.'.$class.' > .slide-background > ul > li > a{
            color:'.$item->background_text_color.';
        }';

        $style[] = '.'.$class.' > .cover > span:before {
            color:'.$item->icon_color.';
        }';
    }
?>
<?php endforeach; ?>

<?php foreach ($style as $css): ?>
    <?php $document->addStyleDeclaration( $css ); ?>
<?php endforeach; ?>


<div id="grid-menu-slider" class="menu-slider">
<?php foreach (array_chunk($menu, 9) as $menu): ?>
<?php $counter = 0; ?>
<?php $count = count($menu); ?>
<div class="grid-box">
    <div class="item-<?php echo $count ?>">
    <?php foreach($menu as $items): ?>
        <?php $counter++; ?>
        <?php if ($items->menu_image) {
                $image = $counter;
                $image_url = $items->menu_image;
                $background = "url(" . $theImage = getImage($image,$image_url,$count) .") no-repeat";
                $shadow = 'shadow';
            } else {
                $shadow = NULL;
                $background = $items->foreground_color.';';
            }
            $class = $items->theme_title;
            ?>
             <?php $linkCount = getLinks($counter,$count); ?>
                <div class="slide<?php echo $items->slide_direction; ?> box-<?php echo $counter ?> <?php echo $class; ?>">
                    <div class="cover <?php echo $shadow; ?>" style="background:<?php echo $background ?>;">
                        <span class="<?php echo 'icon-'.$items->icon; ?> <?php echo $shadow; ?>"><?php echo $items->menu_title; ?></span>
                    </div>
                    <div class="slide-background">
                        <?php foreach (array_chunk($items->submenu, $linkCount) as $items): ?>
                        <ul>
                            <?php foreach($items as $submenu): ?>
                                <?php echo $submenu; ?>
                            <?php endforeach; ?>
                        </ul>
                        <?php endforeach; ?>
                    </div>
                </div>
    <?php endforeach; ?>
    </div>
</div>
<?php endforeach; ?>
</div>
<div class="clearfix"></div>
Improve this question
2
  • What are the "different returns" you mention? Commented Oct 18, 2013 at 9:40
  • well the one switch case returns 12 and then the other switch case returns an image url, im just trying to join them in one switch case Commented Oct 18, 2013 at 9:41

3 Answers 3

Reset to default
2

I dont know how to make this better, but I can suggest an alternative.

You can simply use an associative array. For egs.:

$images[1][1] = "images/menu_slider/960x200/%s";
$images[1][2] = "images/menu_slider/595x200/%s";

and so on...

Then, to access the image with $image as 1 and $count as 2, you could simply do:

$url = sprintf($images[$image][$count],basename($image_url));

Hope this helps...

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

I agree with him, and what you can do is instead of $image[1][2], use $image[1][2]['link'] and $image[1][2]['othervalue'] to combine two functions in one.
In case you still want to use a switch statement but return 2 different values, again, you can return an array
and this would be best practice in this situation?
whatever execute fast, make your code readable for debugging is best practise, so yes it is good thing to do, than switch case, just one drawback that if you didn't write this array in function than it take little memory on global space. which doesn't count considering today's hardware.
Could you give me an example of this within a switch case like Sahil Recommended?
2

You can create a function for building each url string :

            if($count == 1){
                return build_url("960x200",basename($image_url));
            }else if($count == 2){

                return build_url("595x200",basename($image_url));
            }else if($count == 6){
                return build_url("480x200",basename($image_url));
            }else{
                return build_url("470x200",basename($image_url));
            }

Your function :

function build_url($size,$img_url)
{
   return strtr("images/menu_slider/{size}/{img_url}",array('{size}'=>$size,'img_url'=>$img_url));
}

In this way, if you need to remake your url string in the future you will do it one time instead of rewriting each url.

Improve this answer

1 Comment

Agree with Jack... This is the best way to do it!
0

Your code is a bit messy, too much if/elseif/else makes code unreadable. Use arrays to manage that:

function getImage($image,$image_url,$count=null)
{
$imgArray=array(
1=>array(1=>'960x200',2=>'595x200',6=>'480x200','other'=>'470x200'),
2=>array(2=>'365x200',3=>'250x200',4=>'250x200',5=>'250x200',6=>'250x200',7=>'250x300','other'=>'250x400'),
3=>array(3=>'240x200','other'=>'240x100'),
4=>'240x100',
5=>array(5=>'960x100',6=>'480x100',7=>'235x100',8=>'235x200','other'=>'235x141'),
6=>array(6=>'480x100',7=>'235x100',8=>'235x200','other'=>'235x141'),
7=>'240x100',
8=>'240x100',
9=>'470x60'
);

if(!is_array($imgArray[$image]) && isset($imgArray[$image]))
$folder=$imgArray[$image];
elseif(isset($imgArray[$image]) && isset($imgArray[$image][$count]))
$folder=$imgArray[$image][$count];
elseif(isset($imgArray[$image]))
$folder=$imgArray[$image]['other'];
else
return false;

return "images/menu_slider/{$folder}/".basename($image_url);

}

function getLinks($links,$count=null){
$linkArray=array(
1=>12,
2=>array(6=>6,'other'=>12),
3=>array(3=>6,'other'=>3),
4=>3,
5=>array(5=>12,6=>6,7=>3,8=>6,'other'=>4),
6=>array(6=>6,7=>3,8=>8,'other'=>4),
7=>3,
9=>3,
9=>2
);

if(!is_array($linkArray[$links]) && isset($linkArray[$links]))
return $linkArray[$links];
elseif(isset($linkArray[$links]) && isset($linkArray[$links][$count]))
return $linkArray[$links][$count];
elseif(isset($linkArray[$links]))
return $linkArray[$links]['other'];

return false;
}
Improve this answer

3 Comments

Besides, I recommend to always use curly brackets, even if not necessary.
I use autoindent with vim editor so I usually don't care about indention, curly brackets...a matter of style.
Well I don't think it's just a matter of style. I've seen many developers that started without curly brackets, then added another line within the if-statement, and wondered why the programm behaves unexpectedly. It's simply good practice to use'em. No further comments on indention. Look at the code in your answer...

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.