Optimise this if/elseif/else statement

PHP
1

Hi.

I have built a view system, works great but i was wondering if anyone can see a better way to optimise this piece of code as phpStorm does not like the fact the if/elseif have empty bodies so can anyone see a really easy way to make it more optimised. e.g. less if statements, simpler etc. Thanks

        if (startHelper::isLoggedIn() && $getItemById['user_pin'] === $_SESSION['user']['pin']) {
            //'view does not count for own';
        } elseif($this->start->alreadyViewedToday($view)) {
            //'already viewed';
        } else {
            $this->start->insertView($view); // 2 = item // 1 == page
        }
2

So, You can do it all with a single if, no else clauses.

You will need:
(): To wrap terms
||: the “OR” evaluator
!: the “NOT” evaluator.

If your current structure is

if(a) { X }
elseif(b) { Y }
else {
Z
}

and so it reads “If a, X, else if b, Y else Z”.
How do you reverse that, so that Z is in the X position?
(Or perhaps it would be easier to figure out “What must be true to get to Z?”)

3

Ye my brain is pretty clogged with other stuff at the moment.

Something like this?

        if ((startHelper::isLoggedIn() && $getItemById['user_pin'] !== $_SESSION['user']['pin']) || $this->start->alreadyViewedToday($view)) {
            $this->start->insertView($view);
        }
4

you’re… partway there.

“If A, X, else if B, Y, else Z”
for Z to happen, both A and B must be false. Which would be written logically either as “Not A And Not B” or as “Not (A or B)” (by De Morgan’s Law)
Note that I didnt change anything inside of A or B.

5

This?


        if (!(startHelper::isLoggedIn() && $getItemById['user_pin'] === $_SESSION['user']['pin']) || $this->start->alreadyViewedToday($view)) {
            $this->start->insertView($view);
        }

Nah that does not work at all. Wow, i’m usually epic at optimising if statements :sob:

6

You have written “Not (A) Or B”. Check your parenthesis.

7

ye oops. i tried this one initially but it got a red squiggly line but it does work

image
image976×56 5.33 KB

Warning: Operations priority might differ from what you expect: please wrap needed with ‘(…)’. but it does work :smile:

Do you know how i can rmeove that red line tho?

Thanks

8

it’s warning you because you’ve got && and || in a row. Add an extra parenthesis set around A.

1 Like
9

done. Thankyou so much @m_hutley

You truly deserve Member of the Month and sorry for the most embarrassing question ever of optimising an IF statement.

1 Like
10

I’m glad you got there :slight_smile:
The warning is generated because your internal line (the red squiggly bit + the ending) reads as A && B || C. This is potentially ambiguous language, because “A and B or C” said out loud could mean “(A and B) or C” or “A and (B or C)” depending on where you put your inflection. Code can’t use inflection, so it uses/requires parenthesis :wink:

2 Likes
11

Wow. Thanks for that explanation. Helpful and also thankyou goodness for being a student and getting PHPStorm free to help me learn with my mistakes. It’s an amazing application :slight_smile:

Thanks again for your help.

12

Does PHPStorm accept this syntax?

<?php 
  $ok  = startHelper::isLoggedIn() 
      && 
         $getItemById['user_pin'] === $_SESSION['user']['pin'] // ) removed 
      || 
         $this->start->alreadyViewedToday($view)
      ;

// EDIT: with added parenthesis
 $ok  = (
          startHelper::isLoggedIn() 
          && 
          $getItemById['user_pin'] === $_SESSION['user']['pin'] // removed) 
         )
         || 
         $this->start->alreadyViewedToday($view)
      ;


  if (!($ok) {
      $this->start->insertView($view);
  }
die;

I prefer making PHP syntax readable at a glance especially old scripts that were written before lunch :slight_smile:

1 Like
13

Needs 50% more parenthesis for readability :stuck_out_tongue:

1 Like
14

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.