2
\$\begingroup\$

I was recently going through some code that I wrote a while ago as an effort to improve my code.

There was one foreach statement that I was especially proud of the rewrite as I believe it is much more readable than it was before, and I believe it will last longer now.

This code in particular is simply a table body that has a check box (Basically a status check) and a plaintext "task" that was assigned to users stored in a database.

Here is the old code:

<?
    $taskuid = $tasks['uid'];
    $type    = "task";
    foreach($taskData as $key => $value) {
        echo "<tr style='" . (($value['status'] == 1) ? "background-color:green;text-decoration:line-through;" : "") . "'>";
        echo "<td onclick='toggleCheck($key, $taskuid, \"$type\")' style='vertical-align:middle;cursor:pointer;'> <center> " . (($value['status'] == 1) ? "<i data-status='$value[status]' id='$key-task' class='fa fa-lg fa-check-circle-o' aria-hidden='true'></i>" : //Status complete
            "<i data-status='$value[status]' id='$key-task' class='fa fa-lg fa-circle-o' aria-hidden='true'></i>") . " </center> </td>"; //Status incomplete
        echo "<td class='unselectable'>$value[task]</td>";
        echo "</tr>";
    }
?>

Here is the new code:

<?
    foreach($taskData as $key => $value) {
        echo "<tr class='" . (($value['status'] == 1) ? "checkedTR" : "") . "'>";
        echo "  <td onclick='toggleCheck($key, $tasks[uid], \"task\")' valign='middle' class='clickable'>";
        echo "      <center>";
        echo "          <i data-status='$value[status]' id='$key-task' class='fa fa-lg fa-" . (($value['status']) ? "check-circle-o" : "circle-o") . "' aria-hidden='true'></i>";
        echo "      </center>";
        echo "  <td class='unselectable'>$value[task]</td>";
        echo "</tr>";
    }
?>

While writing this, I realized the code actually has the same amount of lines, but there is (what I consider to be) a significant difference in the amount of symbols used (The old code being 752 characters and the new one being 564 characters That's nearly 200 characters!)

My question is this, what would you have done differently while rewriting this code? I'm very interested in the different ways code can be written, I know there are a lot of ways this simple task could be done.

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Consider

<?php
    foreach ($taskData as $key => $value) {
        $class = ($value['status']) ? 'check-' : '';
        $trClass = ($value['status'] == 1) ? ' class="checkedTR"' : '';
        echo <<<EOHTML
<tr$trClass>
  <td onclick="toggleCheck($key, {$tasks['uid']}, 'task')" valign="middle" class="clickable">
    <center>
      <i data-status="{$value['status']}" id="$key-task" class="fa fa-lg fa-{$class}circle-o" aria-hidden="true"></i>
    </center>
  </td>
  <td class="unselectable">{$value['task']}</td>
</tr>
EOHTML;
    }
?>

I prefer the long versions of tags. So no <? or <=. It may not matter as much now, but it used to be that some sites would turn off the short versions. So software could suddenly stop working for no apparent reason.

I prefer to use single quotes in PHP unless I have to use double quotes, e.g. for variable interpolation. This way I can see at a glance that a string is not using variable interpolation.

If I'm working with a block of lines in PHP, I prefer to use Heredocs and Nowdocs. I prefer Nowdoc, but it won't work here. We need the variable interpolation. The original code will put the entire thing on one line. This version will display it more like it looks here.

I prefer to use double quotes in HTML. No particular reason except that it looks better to me.

I prefer to avoid the $tasks[uid] syntax. Outside of a string, it's wrong and unreliable. Inside of a string it's reliable, but it can be misleading. People see it working inside of a string and don't know why the behavior is special. Also, if people copy something like $tasks[FIELD] into a string, it may not be obvious why it doesn't work the same way inside a string as outside. Always using the curly braces is clearer in my opinion.

In this case, I would pick the Javascript quoting to avoid escaping, i.e. single quotes.

Your HTML indentation is inconsistent. Sometimes two spaces and sometimes four.

You lost a closing </td> in the refactor.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your answer :) Your right, I did miss a </td>! I didn't even notice. I didn't realize that you could use itineraries in that way. I was also unaware that you could wrap {}'s around a php variable in the middle of a string, I didn't know that it worked that way. I have also never seen the EOHTML markup before. Is this the Heredocs/Nowdocs you were talking about? Do I need an external tool for that to work? Also, I agree with the using doublequotes in HTML - I tend to use single quotes only when they are inside a php echo. \$\endgroup\$ Jul 5, 2017 at 23:02
  • \$\begingroup\$ I tried to put in your code because I wanted to see how it looked in the actual markup - I like the look of it! However, I dislike with the EOHTML that it seems to require the the ending EOHTML; have no whitespace in front of it. Is this by design, or am I missing something? \$\endgroup\$ Jul 5, 2017 at 23:09
  • \$\begingroup\$ It is by-design. Yes, that is a Heredoc, which is a special way to write a string. Note that the EOHTML is user-specified. \$\endgroup\$
    – mdfst13
    Jul 6, 2017 at 4:04
  • \$\begingroup\$ I actually really dislike that, it makes the formatting look really bad when all the code is 5-6 tabs out and then the last line is 0 tabs. \$\endgroup\$ Jul 6, 2017 at 12:32
  • \$\begingroup\$ Also, why is this considered better than just wrapping the whole thing in double quotes? \$\endgroup\$ Jul 6, 2017 at 12:36
1
\$\begingroup\$

Agreeing with the answer by mdfst13, there are a few other considerations:

Move styles out to CSS

td {
    border: 1px solid #f00;
    padding: 3px;
    text-align: center;
}
.checkedTR {
    background-color:green;
    text-decoration:line-through;
}

with those styles in CSS (either an external CSS file or in a <style> tag in the header) the class attributes can be minimized. Also, the text-align style can reduce the need for the <center> tag (though if there are images or other nested tags, one might need to utilize margin: auto).

Move click handler (on table cell) to JS

I recommend using an Event delegate, like the one below. That way there is a separation of the click logic from the HTML (display layer). And one could use a library like jQuery, VueJS, Angular, etc. to handle many common tasks and browser differences.

The code below listens for clicks on the DOM and checks if the target was the table cell with class name clickable (or a child node) utilizing classlist.contains().

<script type="text/javascript">
    document.addEventListener('DOMContentLoaded', function() {
        document.addEventListener('click', function(clickEvent) {
            var element = clickEvent.target;
                while (element != document) { //ensure we haven't traversed up to the top of DOM
                    if (element.classList.contains('clickable')) { //found clickable table cell
                        //implentation of toggleCheck
                        console.log('clicked on ', event.target);
                        break; //break out of chain
                    }
                    element = element.parentNode; //continue up DOM chain
                }
            });
        })
    </script>

Move table row generation code into function

One can create a function to return the HTML for each table row and use functional programming techniques like array_map() to get an array of table rows and use implode() to join them together.

Note also here we are using Heredoc syntax, as mdfst13 recommends, so double- and single-quotes can be used in-line without the need for escaping.

And there may need to be some re-working to reference $tasks correctly - if you are working with a class definition, that could be moved to a instance/member variable and then have the function be a method in the class that can access that variable.

$html = '<table>';
$html .= implode('', array_map('getTableRow', array_keys($taskData), $taskData));
$html .= '</table>';
function getTableRow($key, $value) {
    $tasks = ['uid'=>'cat'];
    $circleClass = "circle-o";
    if ($value['status']) {
        $circleClass = "check-circle-o";
    }
    return <<<TR
    <tr class="status_{$value['status']}">
        <td data-key="$key" data-task-uid="{$tasks['uid']}" valign="middle" class="clickable">
            <i data-status="{$value[status]}" id="task-$key" class="fa fa-lg fa-{$circleClass}" aria-hidden='true'></i>
        <td class="unselectable">{$value['task']}</td>
    </tr>
TR;
}

Also note that the class for the table row is simplified to simply status_{$value['status']}, and consequently the CSS selector (i.e. .checkedTR) should be updated accordingly.

See a demonstration of this in this playground example.

\$\endgroup\$
0

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.