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. $vars
signifies 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.
1 Answer 1
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
}
-
\$\begingroup\$ Some really great things here, thanks for all your effort. This gives me a lot to think about! \$\endgroup\$Danny Englander– Danny Englander2016年03月17日 14:07:23 +00:00Commented Mar 17, 2016 at 14:07
-
\$\begingroup\$ I have added a better issue summary now. \$\endgroup\$Danny Englander– Danny Englander2016年03月17日 14:18:09 +00:00Commented Mar 17, 2016 at 14:18