Multiple condition IF statement
I've got an if statement with multiple conditions that I just can't seem to get right
if(((ISSET($_SESSION['status'])) && ($_SESSION['username']=="qqqqq")) ||
((ISSET($_SESSION['status'])) && ($_SESSION['company']=="wwwww开发者_如何学Gow")) ||
((ISSET($_SESSION['status'])) && ($_SESSION['company']=="ffffffff")) ||
((ISSET($_SESSION['status'])) && ($_SESSION['company']=="ggggggg")) ||
((ISSET($_SESSION['status'])) && ($_SESSION['company']=="hhhhhhh")) ||
($_SESSION['LOGINTYPE']=="ADMIN")) {
Its supposed to return true if the status is set as well as either one of those company names OTHERWISE if the logintype is admin it should also return true regardless of company or status
What about this :
if (
(isset($_SESSION['status']) && $_SESSION['username']=="qqqqq")
|| (isset($_SESSION['status']) && in_array($_SESSION['company'], array('wwwwww', 'ffffffff', 'ggggggg', 'hhhhhhh')))
|| $_SESSION['LOGINTYPE']=="ADMIN"
)
Rewriting the condition to try to make it a bit more easier to read might help :
- One possible case per line :
- status set and username OK
- status set and company OK
- admin
- Using in_array instead of several tests on company -- I think it's easier to understand
At the very minimum, rework it so it reads what you said it should do:
Its supposed to return true if the status is set as well as either one of those company names OTHERWISE if the logintype is admin it should also return true regardless of company or status
function mayAccessResource(array $sessionState)
{
return (hasStatus($sessionState) && isAllowedUserOrCompany($sessionState))
|| isAdmin($sessionState);
}
Then add methods for those encapsulating the 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';
}
That's still far from perfect because it hardcodes way too much information, but at least it is much more readable now. Since all of these operate on the session State, it would be much prettier there was an object SessionState. Even better would be to tease the various possible session states further apart and have a Company, User and AccessControl class.
Also note the comments below your question.
Some formatting would make that a lot easier to parse mentally =) Plus you probably could simplify the companies aspect of things by creating an array of permitted companies. Testing one at a time makes this if() difficult to maintain and read - each time you add a company you risk breaking the logic.
Also, it looks like if $_SESSION['LOGINTYPE'] == 'ADMIN'
, this is an override which should always be true, while otherwise you always need $_SESSION['status'] to be set.
$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) {
...
}
Another option:
if (!$_SESSION['LOGINTYPE']=="ADMIN") {
return false;
}
if (!ISSET($_SESSION['status']) {
return false
}
if ($_SESSION['username']=="qqqqq") {
return true;
}
// etc
Or use switch starting from the the third conditional.
Even better would be to make this object oriented:
if ($this->user->isAdmin()) || $this->user->belongsToCompany($this->allowedCompanies)
This reads even better through the creation of a 'higher level language'.
Note: it's not about getting the least character on the as few lines as possible.
Why don't you use try-catch-blocks and set a variable to true if a case becomes true?
It's much clearer to read and sustain.
try($foo) {
catch x:
$bar = true;
default:
$bar = false;
}
精彩评论