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\$

closed as off-topic by Mast, t3chb0t, Stephen Rauch, Toby Speight, Sᴀᴍ Onᴇᴌᴀ Oct 4 '18 at 15:37

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Mast, t3chb0t, Stephen Rauch, Toby Speight, Sᴀᴍ Onᴇᴌᴀ
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\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');
\$\endgroup\$

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