IF statement with multiple conditions

Question

I have an if statement with several conditions that I just can't get right

if(((ISSET($_SESSION['status'])) && ($_SESSION['username']=="qqqqq")) || ((ISSET($_SESSION['status'])) && ($_SESSION['company']=="wwwwww")) || ((ISSET($_SESSION['status'])) && ($_SESSION['company']=="ffffffff")) || ((ISSET($_SESSION['status'])) && ($_SESSION['company']=="ggggggg")) || ((ISSET($_SESSION['status'])) && ($_SESSION['company']=="hhhhhhh")) || ($_SESSION['LOGINTYPE']=="ADMIN")) { 

It is supposed to return true, if the status is set as one of these OTHERWISE company names, if logintype is an administrator, it should also return true regardless of company or status

Answer 1

How about this:

 if ( (isset($_SESSION['status']) && $_SESSION['username']=="qqqqq") || (isset($_SESSION['status']) && in_array($_SESSION['company'], array('wwwwww', 'ffffffff', 'ggggggg', 'hhhhhhh'))) || $_SESSION['LOGINTYPE']=="ADMIN" ) 

Overwriting a condition to try to make it easier to read can help:

• One possible case for each row:
  • status and username OK
  • status and company OK
  • admin
• Using in_array instead of several tests for the company - I think it’s easier to understand

Answer 2

At a minimum, recycle it so that it reads what you said it should do:

It is supposed to return true, if the status is set as one of these OTHERWISE company names, if logintype is an administrator, it should also return true regardless of company or status

 function mayAccessResource(array $sessionState) { return (hasStatus($sessionState) && isAllowedUserOrCompany($sessionState)) || isAdmin($sessionState); } 

Then add methods to encapsulate single tests:

 function hasStatus($sessionState) { return isset($sessionState['status']); } function isAllowedUserOrCompany($sessionState) { return isAllowedUser($sessionState) || isAllowedCompany($sessionState); } function isAllowedUser($sessionState) { return isset($sessionState['user']) && $sessionState['user'] === 'blah'; } function isAllowedCompany($sessionState) { $allowedCompanies = array('foo', 'baz', 'baz'); return isset($sessionState['company']) && in_array($sessionState['company'], $allowedCompanies); } function isAdmin($sessionState) { return isset($sessionState['LOGINTYPE']) && $sessionState['LOGINTYPE'] === 'admin'; } 

This is still far from perfect, because it hardcodes too much information, but at least now it is much more readable. Since they all work in session state, it would be much more beautiful to have a SessionState object. It would be even better to tease the various possible session states further apart and have a Company, User, and AccessControl class.

Also pay attention to the comments below your question.

Answer 3

Some formatting would make it much easier to parse mentally =) In addition, you could probably simplify the aspect of the company by creating many authorized companies. Testing one at a time does this if () it is difficult to maintain and read - every time you add a company, you run the risk of breaking the logic.

Also, it looks like if $_SESSION['LOGINTYPE'] == 'ADMIN' , this is an override that should always be true, otherwise you always need to set $ _SESSION ['status'].

 $companies = array( 'wwww'=>true, 'ffff'=>true, 'gggg'=>true, 'hhhh'=>true ); if ( $_SESSION['LOGINTYPE'] == 'ADMIN' || isset($_SESSION['status']) && ( $_SESSION['username'] == 'qqqqq' || isset($companies[$_SESSION['company']]) ) ) { ... } 

Or maybe more clearly:

 $pass = false; if ($_SESSION['LOGINTYPE'] == 'ADMIN') { $pass = true; } else if (isset($_SESSION['status'])) { if ($_SESSION['username'] == 'qqqq') $pass = true; if (isset($companies, $_SESSION['company']) $pass = true; } if ($pass) { ... } 

Answer 4

Another option:

 if (!$_SESSION['LOGINTYPE']=="ADMIN") { return false; } if (!ISSET($_SESSION['status']) { return false } if ($_SESSION['username']=="qqqqq") { return true; } // etc 

Or use the switch starting with the third legend.

It would be even better to make this object oriented:

 if ($this->user->isAdmin()) || $this->user->belongsToCompany($this->allowedCompanies) 

This is improved by creating a higher level language.

Note: this is not about getting the smallest character on as few lines as possible.

Answer 5

Why don't you use try-catch blocks and set the variable to true if the case becomes true?

It is much clearer to read and maintain.

 try($foo) { catch x: $bar = true; default: $bar = false; }