0
\$\begingroup\$

I wrote this code and I am wondering if there is a better way to phrase this. I was considering switch case but I was not sure how to translate this code into that.

The purpose of this code is to add a css body class to the webpage depending on if certain block regions show up on the page or not. $varssignifies a path to an array available within the function. So for example preface_first is a block region within the page and I am testing to see if it is present or not. If it is, then we add an HTML body class i.e. $vars['attributes']['class'][] = for example.

/**
 * Override or insert variables into the page template.
 */
function template_preprocess_html(&$vars) {
 // Preface columns.
 if (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_second']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-three';
 }
 elseif (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_second'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 elseif (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 elseif (!empty($vars['page']['preface_second']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 else {
 $vars['attributes']['class'][] = 'preface-one';
 }
}

The code works as is but I somehow think it can be improved to be shorter and more efficient.

asked Mar 16, 2016 at 22:55
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

You really do need to explain the overall purpose of your code, where this $var comes from and where this is going.

You might be suprised at the insights and thoughts you get in response.

But if we limit it to just the following code, here are my thoughts:

// Preface columns.
 if (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_second']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-three';
 }
 elseif (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_second'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 elseif (!empty($vars['page']['preface_first']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 elseif (!empty($vars['page']['preface_second']) && !empty($vars['page']['preface_third'])) {
 $vars['attributes']['class'][] = 'preface-two';
 }
 else {
 $vars['attributes']['class'][] = 'preface-one';
 }

More efficient is a bit vague, especially as time and memory performance wouldnt differ enough to worry about unless you were iterating over this a great many times.

But with regards to readability, its largely about consistency and purpose clarity. Your writing style WILL change over time, sometimes in drastic ways but mostly a matter of verbosity and style.

There is nothing wrong with the above code per-say, but it is a bit 'cluttered', so readability can be improved by shortening. Exactly how far depends on you at this time, as consistency and personal readability are also key, especially for others or yourself having to read this code later on for any reason.

Im not saying this is the best way to do it, but personally I might try something like this to make it shorter and more efficient:

$checks = [!empty($vars['page']['preface_first']), !empty($vars['page']['preface_second']), !empty($vars['page']['preface_third'])];
$classes = [];
// other code
$vars['attributes']['class'] = array_merge($vars['attributes']['class'],$classes);

Then maybe one of the following flavours, largely depending on whether you expect this to grow more complicated and what seems more clear to you:

very explicit/verbose:

if(!empty($checks[0]) && !empty($checks[1]) && !empty($checks[2]))
{
 $classes[] = 'preface-three';
}
elseif(!empty($checks[0]) && !empty($checks[1]))
{
 $classes[] = 'preface-two';
}
elseif(!empty($checks[0]) && !empty($checks[2]))
{
 $classes[] = 'preface-two';
}
elseif(!empty($checks[1]) && !empty($checks[2]))
{
 $classes[] = 'preface-two';
}
else
{
 $classes[] = 'preface-one';
}

more compact:

if(!empty($checks[0]) && !empty($checks[1]) && !empty($checks[2]))
{
 $classes[] = 'preface-three';
}
elseif(!empty($checks[0]) && (!empty($checks[1]) || !empty($checks[2])))
{
 $classes[] = 'preface-two';
}
elseif(!empty($checks[1]) && !empty($checks[2]))
{
 $classes[] = 'preface-two';
}
else
{
 $classes[] = 'preface-one';
}

very compact:

if(!empty($checks[0]) && !empty($checks[1]) && !empty($checks[2]))
{
 $classes[] = 'preface-three';
}
elseif((!empty($checks[0]) && (!empty($checks[1]) || !empty($checks[2]))) || (!empty($checks[1]) && !empty($checks[2])))
{
 $classes[] = 'preface-two';
}
else
{
 $classes[] = 'preface-one';
}

There is also the ternary operator, which i do use frequently when there is one condition, but rarely when there are many.

http://php.net/manual/en/language.operators.comparison.php

$classes[] = (!empty($checks[0]) && !empty($checks[1]) && !empty($checks[2])) 
 ? 'preface-three' 
 : ((!empty($checks[0]) && (!empty($checks[1]) || !empty($checks[2]))) || (!empty($checks[1]) && !empty($checks[2])))
 ? 'preface-two'
 : 'preface-one';

As a side note, there is also something like the following, but i dislike it as it doesnt feel natural to me, so i stopped before finishing it

if(!empty($checks[0]))
{
 if(!empty($check[1]))
 {
 if(!empty($checks[2]))
 {
 $classes[] = 'preface-three';
 }
 else
 {
 $classes[] = 'preface-two';
 }
 }
}
elseif(!empty($checks[1])
{
 // ect. ect. - not recommended in my mind
}

answered Mar 17, 2016 at 4:13
\$\endgroup\$
2
  • \$\begingroup\$ Some really great things here, thanks for all your effort. This gives me a lot to think about! \$\endgroup\$ Commented Mar 17, 2016 at 14:07
  • \$\begingroup\$ I have added a better issue summary now. \$\endgroup\$ Commented Mar 17, 2016 at 14:18

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.