2

I'm currently working on a foreach loop with nested if statements but I'm pretty sure there's a better way of writing these chunks of if statements.
I found this post: PHP if shorthand and echo in one line - possible?
Though this post is for single conditions, I would like to write mine in the same way(single lined).

I'm not that experienced in PHP myself so I'm sort of stuck on doing it the old fashioned way:

   if(($colorLevel['name'] === 'ATTR_VPMCV13') && ($colorLevel['level'] >= 80))
    {
        $prominentSideNumberArray[] = 10;
    }

   elseif(($colorLevel['name'] == 'ATTR_VPMCV13') && ($colorLevel['level'] >= 60) && ($colorLevel['level'] <= 70)){
        $prominentSideNumberArray[] = 8;
    }

If someone could properly explain what code to use where and why, that could really help me, and/or others, out. I've looked at the manual but I just can't figure out what to use where.

Dannylycka
  • 33
  • 1
  • 9
  • Just a little comment nothing to do with your question, but do not use strings like 'ATTR_VPMCV13' when comparing something. I do not know where you get that value from, but if you were to change something in a database, you would have to hard code all those strings instead of having it as 1 variable. – Tomm Oct 03 '17 at 06:33
  • 1
    @Tomm, or you could define it as a constant in a single location. – KIKO Software Oct 03 '17 at 06:34
  • also a comment, it's the name of the toner the company uses. it's hard programmed into multiple aplications. – Dannylycka Oct 03 '17 at 06:35
  • @KIKOSoftware that's something i haven't considered yet, thank you! – Dannylycka Oct 03 '17 at 06:35
  • Thats basically what I explained just make sure to define it no matter how you get the value. – Tomm Oct 03 '17 at 06:36

2 Answers2

3

There is no such thing like an "if shorthand".
?: is an operator, if is a control structure. They are different language concepts that have different purposes and do different things. An expression that contains the ternary conditional operator (?:) can always be rewritten as two expressions and an if/else statement. The vice-versa is usually not possible.


The code you posted can be written to be much easier to read if you extract the common checking of $colorLevel['name'] into a separate if that includes the rest of the tests, extract $colorLevel['level'] into a new variable with shorter name and make the conditions that use $colorLevel['level'] use the same rule:

$level = $colorLevel['level'];
if ($colorLevel['name'] == 'ATTR_VPMCV13') {
    // Don't mix '<=' with '>=', always use '<='...
    if (60 <= $level && $level <= 70) {
        $prominentSideNumberArray[] = 8;
    // ... and put the intervals in ascending order
    } elseif (80 <= $level) {
        $prominentSideNumberArray[] = 10;
    }
}

If there are multiple if statements that verify different values of $colorLevel['name'] then the intention is more clear if you use a switch statement:

$level = $colorLevel['level'];
switch ($colorLevel['name'])
{
    case 'ATTR_VPMCV13':
        if (60 <= $level && $level <= 70) {
            $prominentSideNumberArray[] = 8;
        } elseif (80 <= $level) {
            $prominentSideNumberArray[] = 10;
        }
        break;

    case '...':
        // ...
        break;

    default:
        // ...
        break;
}
insertusernamehere
  • 23,204
  • 9
  • 87
  • 126
axiac
  • 68,258
  • 9
  • 99
  • 134
0

You can achieve this by using a ternary operator. Look at the following code:

$prominentSideNumberArray[] = ((($colorLevel['name'] === 'ATTR_VPMCV13') &&
 ($colorLevel['level'] >= 80) )? 10 : (($colorLevel['name'] == 'ATTR_VPMCV13') && 
($colorLevel['level'] >= 60) && ($colorLevel['level'] <= 70)?8:""))  ;

EDIT As per comments and you have to compare same value its better to define name

$color_name = "ATTR_VPMCV13";
if($colorLevel['name'] == $color_name )
    $prominentSideNumberArray[] = (($colorLevel['level'] >= 80)? 10 : (
($colorLevel['level'] >= 60) && ($colorLevel['level'] <= 70)?8:""))  ;

DEMO with different approach

EDIT

Keep in mind that this solution is less readable than if-else statement.

Community
  • 1
  • 1
B. Desai
  • 16,414
  • 5
  • 26
  • 47
  • 4
    although this is what op asked for, in my opinion this is way less readable than the original if-elseif statement. I would stick with if-elseif for sure – kscherrer Oct 03 '17 at 06:40
  • Nice answer, if @Dannylycka defines 'ATTR_VPMCV13' as a variable somewhere it would be even better. And also more secure – Tomm Oct 03 '17 at 06:40
  • nice answer indeed! I'll be sure to declare the toner somewhere to improve my code! – Dannylycka Oct 03 '17 at 06:42
  • Could you perhaps also explain what you write? for instance, why is there a "" at the end? as it doesnt close anything or declare anything. – Dannylycka Oct 03 '17 at 06:47
  • I also agree that you should only use this operator only if you cannot add an if block, or the conditions are simple. Why? One is readability, as others said, and just imagine adding later more conditions and see how `beautiful` will look. It becomes hard to track, more error prone, you have no format, and so on. – Badea Mihai Florin Oct 03 '17 at 06:47
  • @Dannylycka `""` only will assign if all conditions become false – B. Desai Oct 03 '17 at 06:49
  • why it's get downvoted ? @Downvoter can you please explain .so that i can correct my mistake – B. Desai Oct 03 '17 at 06:49
  • It doesn't produce the same result as the original code. There is no case when the original code stores an empty string in `$prominentSideNumberArray`. – axiac Oct 03 '17 at 06:52
  • what is the difference in result @axiac? I have tested it. check link https://3v4l.org/3kLvv – B. Desai Oct 03 '17 at 06:53
  • I would also like to echo the first comment - this code (IMHO) is less maintainable and almost unreadable. It may be neat to shorten the code in this way - but having a clearly structured if/else structure makes it much easier to understand and more likely to work. – Nigel Ren Oct 03 '17 at 07:18
  • @NigelRen I only have provide solution for what OP is asking for. I also know it is less readable than if-else – B. Desai Oct 03 '17 at 07:19
  • I would also like to note that in general if all you need is to assign value based on combo of static parametrs, it would be much cleaner and maintainable to do store the combos in a lookup table, for example, promSideNums['ATTR_VPMCV13'][60]=8. Then you need almost no ifs, just plug in colorlevel names and levels as keys. – Gnudiff Oct 03 '17 at 07:24
  • i did mention the following: '' I'm not that experienced in PHP myself '' – Dannylycka Oct 03 '17 at 07:39
  • For my case answer of @axiac's answer is correct. I have tried your approach for php but it is not assigning the correct value. – rahim.nagori Jul 29 '19 at 07:29