0
\$\begingroup\$

This is a fairly short code snippet, but it made me a little headache, because it is hard to understand, isn't it?

// You have to be in both 'group' and 'board' to get access (or be a superuser)
if ( ( !$is_in_group || !$is_in_board ) && !$is_superuser) {
    die('No access');
}

This code is from a download page, where registered users can download files. Only users within a certain board and group are allowed to download files. The three variables are just for this demo. The values are coming from a CMS function.

Do you have a more readable or better way to verifying access here?

\$\endgroup\$
  • \$\begingroup\$ It's lacking context, but simplified like that I don't see anything wrong with it. \$\endgroup\$ – Mast Oct 4 '18 at 11:05
  • 1
    \$\begingroup\$ Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please take a look at the help center. \$\endgroup\$ – Mast Oct 4 '18 at 11:07
5
\$\begingroup\$

You can rewrite it like this:

// you have to be in both 'group' and 'board' to get access
$has_access = $is_in_group && $is_in_board;
// you must have access or be a superuser
if (!($has_access || $is_superuser)) die('No access');

I don't think an explanation is needed.

Alternatively you could even write:

// you have to be in both 'group' and 'board' to get access
$has_access = $is_in_group && $is_in_board;
// you must have access or be a superuser
if ($has_access || $is_superuser) {
  // ....
  // do whatever requires access
  // ....
} else die('No access');
| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.