1
\$\begingroup\$

I was recently going through and refactoring old code, and I eventually came across the code for my website's menu bar, so I started to refactor it.

I also recently was introduced to Heredocs, which I am very new to and not sure all the ins-and-outs yet.

Here is the refactored code for part of it (I am mostly interested in responses about this specific part of the menu, which is a dropdown menu that every logged in user gets that essentially has their name and then it drops down into some other options regarding themes):

<?php

    $showAdminPanel = ((hasPerms($_SESSION['users_perms'], $_SESSION['role_perms'], 8)) ? "<li><a href='{$backwards}../admin/'>Admin Panel</a></li>":"");
    $devPerms = ((hasPerms($_SESSION['users_perms'], $_SESSION['role_perms'], 13)) ? "<li>
                                                                                        <li><a href='{$backwards}development/issues/index.php'>Issues</a></li>
                                                                                        <li><a href='{$backwards}development/requests/index.php'>Requests</a></li>
                                                                                        <li><a href='{$backwards}development/changes.php'>Changelog</a></li>
                                                                                      </li>":"");

    $themes = "";
    foreach(scandir($assetsDir."themes/") as $file) {
        if((strcmp($file,".") != 0) && (strcmp($file,"..") != 0)) {
            $themes .= '<li><a class="theme" href="#" rel="'.$themePath."/".$file.'">'.substr(ucfirst($file),0,-4).'</a></li>';
        }
    }

    echo <<<USERACCOUNTDROPDOWN
        <li class="menu-item dropdown">
            <a href="#" class="dropdown-toggle" data-toggle="dropdown">{$_SESSION["user"]["name"]}<b class="caret"></b></a>
            <ul class="dropdown-menu">
                {$showAdminPanel}
                <li id="nav" class="menu-item dropdown dropdown-submenu">
                    <a href="#" class="dropdown-toggle" data-toggle="dropdown">Themes</a>
                    <ul class="dropdown-menu">
                        <li><a class="theme" href="#" rel="{$themePath}/cosmo.css">Default: Cosmo</a></li>
                        <li role="separator" class="divider"></li>
                        {$themes}
                    </ul>
                </li>
                <li id="nav" class="menu-item dropdown dropdown-submenu">
                    <a href="#" class="dropdown-toggle" data-toggle="dropdown">Development</a>
                    <ul class="dropdown-menu">
                        {$devPerms}
                    </ul>
                </li>
                <li role="seperator" class="divider"></li>
                <li><a href="{$backwards}login/index.php">Logout</a></li>
            </ul>
        </li>
USERACCOUNTDROPDOWN;
?>

The reason that I am more interested in this section of the code rather than the entire menu bar is that, this part of it has some permissions baked in using the $devPerms variable, basically if user has perm ID 13 it creates a string which has links to specific parts of the website, if user does not, the string is blank.

This code works, but I feel like there is a better way to accomplish what I am trying to do here that will make it look cleaner.

My question is this: How would you have written this code? I would love to improve my Heredocs understanding as I believe it will improve my programming in general.

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

I wrestled a bear once has mentioned many great points and it is difficult to find other areas that could be changed.

There isn't much I would change about the text in the heredoc string. The biggest thing that I really see that could be done differently is that not all variables need to be wrapped in a complex expression - so the curly braces could be removed for all variables except for {$_SESSION["user"]["name"]} and {$backwards}login. The others can be simplified like rel="$themePath/cosmo.css".

And as I wrestled a bear once stated, it might be simpler to use an array to create the list of theme files. Going a couple steps further with that notion, a functional approach could be taken with array_map(), which yields an array and then that array could be passed to implode(). Notice the removal of the dot items is simplified using this trick from dwieeb at gmail dot com (via the PHP documentation).

$themes = implode('', array_map(function($file) {
        return '<li><a class="theme" href="#" rel="'.$themePath."/".$file.'">'.substr(ucfirst($file),0,-4).'</a></li>';
    },array_diff(scandir($assetsDir."themes/"), array('..', '.'))));

See a demonstration of this in this playground example.

\$\endgroup\$
2
  • \$\begingroup\$ I like complex expression syntax not because it's NEEDED, but because it makes variables look cleaner, and it's easier to see what's going on when you don't have a fancy syntax highlighter \$\endgroup\$ Aug 30, 2017 at 15:47
  • \$\begingroup\$ Good point- readability is an important aspect! \$\endgroup\$ Aug 30, 2017 at 15:50
1
\$\begingroup\$

Excessive perens in your ternary - they don't help with readability, scrap em...

$showAdminPanel = hasPerms($_SESSION['users_perms'], $_SESSION['role_perms'], 8) ? "<li><a href='{$backwards}../admin/'>Admin Panel</a></li>" : "";

For long ternary lines I sometimes like to put a line break and a tab before the ? and :.

$showAdminPanel = hasPerms($_SESSION['users_perms'], $_SESSION['role_perms'], 8) 
    ? "<li><a href='{$backwards}../admin/'>Admin Panel</a></li>" 
    : "";

The second ternary operator contains invalid HTML. <li> must be a child of <ul> or <ol>, not another <li>.

Using strcmp seems like overkill in the scandir loop. it would be faster to just use an == operator. Also, it's generally better practice to creatte and append to an array than to a string. Every time you add to taht string in that loop, the entire string has to be loaded into memory all over again. It's much quicker to use a buffer liek this...

$themes = array();
foreach(scandir($assetsDir."themes/") as $file) {
    if($file == "." || $file == "..") continue;
    $themes[] = '<li><a class="theme" href="#" rel="'.$themePath."/".$file.'">'.substr(ucfirst($file),0,-4).'</a></li>';
}
// Turn the buffer into a string
$themes = implode('', $themes);

There is no reason for a heredoc here since you're at the end of the script. Just close the php tag and put the stuff after that (replacing the curlies with <?php= and ;?>), that way you don't have the PHP interpreter interpreting things it doesn't need to interpret.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your answer! I agree with the parenthesis in the ternaries, thank you. I also like the way the lines are broken in your example. However, it does not contain invalid HTML. As you can see in my code the ternary is then inserted into the heredocs code, there is more to this heredocs code than just the theme picker. It's valid HTML because the variable is inserted into the menu between a <ul> element. I used heredoc because the entire heredoc is an output, and an important one. \$\endgroup\$ Jul 11, 2017 at 12:44
  • \$\begingroup\$ Look at your code, Crouton.. <li><li><a .... even if you do put that in a <ul> you still have an <li> inside another <li>. A heredoc is not required to output. PHP isn't even required to output. Using PHP heredoc to output something does not make it safer or more efficient. I've noticed you like to argue so that's all I'm going to say on this post. \$\endgroup\$ Jul 11, 2017 at 12:50
  • \$\begingroup\$ You're totally right actually I didn't even realize I had the <li> and the </li> in the ternary. I was looking in the heredocs section, and I was saying that it was valid HTML there - but again you are right. As for the output part, I know that. But the output is more than just the theme selection, it's several other things. I don't want to break apart my heredocs into more sections to include php in the middle of it, because I feel as though that would be extremely messy. Also, if you took my tone as argumentative I apologize, I'm just having a conversation, and sometimes I come off wrong \$\endgroup\$ Jul 11, 2017 at 13:04
  • \$\begingroup\$ It happens. Good luck with your code. \$\endgroup\$ Jul 11, 2017 at 13:14

Your Answer

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

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