CODEWITHSUNDEEP
php
JellyAce
JellyAce

Reputation: 96

How to simplify this kind of code?

This code is properly working. However, I'm thinking of ways to shorten it since I'm still learning PHP. I'm using this snippet to generate a title for an e-mail I am going to send to somebody.

foreach($_POST as $key => $value) {
    if(strpos($key,'cloud') !== false && !$a) {
        $title .= "CS ";
        $a = true;
    }

    if(strpos($key,'dco') !== false && !$b) {
        $title .= "DC ";
        $b = true;
    }

    if(strpos($key,'comm') !== false && !$c) {
        $title .= "BC ";
        $c = true;
    }

    if(strpos($key,'fiber') !== false && !$d) {
        $title .= "FC ";
        $d = true;
    }
}

I'm pretty sure they do the same thing (all of the if statements). If there's anything you can suggest/advice, please let me know!

Cheers!

Upvotes: 0

Views: 89

Answers (3)

Linblow
Linblow

Reputation: 479

Another solution without using a function:

    $infos = array(
        array('cloud', 'CS', &$a),
        array('dco', 'DC', &$b),
        array('comm', 'BC', &$c),
        array('fiber', 'FC', &$d)
    );

    foreach($_POST as $k => $v) {
        foreach($infos as $info) {
            if ( !$info[2] && strpos($k, $info[0]) !== false) {
                $title .= $info[1]. ' ';
                $info[2] = true;
            }
        }
    }

This isn't the best way though. As suggested in another answer, it'd be better to use a function: an anonymous function (aka closure).

Upvotes: 0

Marcin Nabiałek
Marcin Nabiałek

Reputation: 111839

You can simple create function. This code should work:

foreach($_POST as $key => $value) {
    checkstrpos($key,'cloud',$a,$title,"CS ");
    checkstrpos($key,'dco',$b,$title,"DC ");
    checkstrpos($key,'comm',$c,$title,"BC ");
    checkstrpos($key,'fiber',$d,$title,"FC ");
}


function checkstrpos($key,$text, &$variable, &$title, $add) {
    if(strpos($key,$text) !== false && !$variable) {
        $title .= $add;
        $variable = true;
    }        
}

Another solution without using references:

foreach($_POST as $key => $value) {
    list($title, $a) = checkstrpos($key,'cloud',$a,$title,"CS ");
    list($title, $b) = checkstrpos($key,'dco',$b,$title,"DC ");
    list($title, $c) = checkstrpos($key,'comm',$c,$title,"BC ");
    list($title, $d) = checkstrpos($key,'fiber',$d,$title,"FC ");
}


function checkstrpos($key,$text, $variable, $title, $add) {
    if(strpos($key,$text) !== false && !$variable) {
        $title .= $add;
        $variable = true;
    }   
    return array($title, $variable);     
}

Upvotes: 2

thomasrutter
thomasrutter

Reputation: 117343

To me I think that code is simple enough.

If you're attempting to make the code shorter, you risk making it harder to read.

The one comment I have is don't use generic variable names like $a, $b, $c and $d. Use proper descriptive variable names instead.

It's possible that you could swap the positions of these conditions, that is, change 

if(strpos($key,'dco') !== false && !$b)

to

if(!$b && strpos($key,'dco') !== false)

But this is more of a micro-optimization than a simplification or change in readability.

Upvotes: 0

Related Questions