Take Only Pictures; Leave Only Footprints

If you've ever visited a National Park or many state parks in the United States, or been involved with a nature-oriented community group, you've likely heard that saying many times. For those who haven't, the meaning should be obvious: leave the shared place that you're moving through in at least as good condition as you found it, so that the next people to travel that way can enjoy it as much as you did.

That applies to environments other than the great outdoors, of course. The phrase popped into my mind earlier today as I was poking at a jQuery plugin to add a context menu to a Web app I'm developing.

It's great that there's so many free software tools out there; I've been using them for more than 25 years, and I've developed more than a few. Sometimes, however, one is reminded of the wisdom of Oliver Wendell Holmes when he said, "Learn from the mistakes of others… you can't live long enough to make them all yourself." But in the grand software tradition, we often give it our best effort.

This is a reasonably well-known plugin, one of several that offer to solve a common problem in more-or-less similar fashion. And if you use it precisely in the way that the author expected, it does the job. But even then, if you're using it in a Web app or site that customers are paying money for, you might hope that they never hit the "View Source" menu item in their browser. And, to be fair, this is not intended as a slam against Matt Kruse or his code; most of the other plugins I've looked at in the last couple of days have at least as many "quirks" and assumptions.

"It's just a context menu," I hear you say. "What could possibly go wrong?…go wrong?…go wrong?…(sound of tape snapping)" (with apologies to Westworld, a movie with many lessons on the art and craft of software development.)

If you're adding the menu to your page when it's first loaded, and you want a static menu, with the options the same for all uses of the context menu, fine. If you want a more dynamic menu, and don't mind attaching functions to the menu that know enough of the detailed state of your page/app that they can generate the properly-adjusted menu items each time the menu is displayed, that works, too. But the main assumptions are that the plugin's main method will be called once and only once on a page, and that you really don't care about the markup being added to the end of your page. The first assumption, particularly in a rich front end, is likely to be naïve; the second is, bluntly, an assault on your professionalism.

What's so bad? Here's the plugin's output for a very simple menu, reformatted for clarity:

<table cellspacing="0" cellpadding="0" style="display: none;">
  <tbody>
    <tr>
      <td>
        <div class="context-menu context-menu-theme-vista">
          <div class="context-menu-item " title="">
            <div class="context-menu-item-inner" style="">One</div>
          </div>
          <div class="context-menu-item " title="">
            <div class="context-menu-item-inner" style="">Two</div>
          </div>
          <div class="context-menu-item " title="">
            <div class="context-menu-item-inner" style="">Three</div>
          </div>
        </div>
      </td>
    </tr>
  </tbody>
</table>

<div class="context-menu-shadow" style="display: none; position: absolute; z-index: 9998; opacity: 0.5; background-color: black;"></div>

If you read HTML and CSS, you're probably nodding your head, thinking "that looks simple enough; hey, hang on a minute…" That "hang on" is where you start seeing any of several issues:

1. We have a table, containing a (rarely-used) tbody, containing a single tr, containing a single td, containing a series of nested divs which contain the menu items. If he'd implemented the entire menu as a table, he might have been able to claim that he was trying to support 1997-era browsers. (Would jQuery 1.4 work in Netscape Navigator 4?) Failing that, it's a mess. Nesting divs (or more semantically appropriate elements) has been a Solved Problem for a decade or so.

2. Even though this is implicitly intended to be fire-and-forget markup added to the DOM at page-creation time, it's disturbing that there are no identifiers for any of the table elements. If you wanted to, say, remove the table later, you'd have to use code like

   $('.context-menu').parent().parent().parent().parent().remove()

   Ick. And then you'd have to go clean up the shadow div separately.

3. Speaking of the shadow div being separate from the menu-enclosing table, why not wrap both in an (identified) div, so that you (and your clients' code) can treat the menu as a single entity?

4. Doing that would also let you dynamically delete and replace the menu, based on changes in the state of your app, without needing to define menu-item handler functions that violate the Principle of Least Knowledge.

5. Another benefit of that would be if your context menu was generated more than once (possibly because the content needed changing or items needed disabling), you'd never have more than one copy of the menu in the DOM. As it is now, you can have an arbitrarily large number, with only the most recently-added being the "active" one. Ugh. This would be more likely to happen on a dev box running a test suite, but still. Ugh.

6. Particularly in a behaviour-driven or test-driven development (BDD or TDD) environment, being able to test/validate the markup and the item-handler logic separately as well as together is important. Doing so with this plugin (and again, to be fair, most of the others) eliminates the normal-use workflow from consideration.

7. One feature of this particular plugin is that it supports having you define your own HTML for the menu and pass it in. But the example given is too simple to be useful as a menu. Browsing the plugin source code seems to indicate that there is no event-handler support for menus defined in this manner; you'd have to iterate through your context menu items, assigning event handlers for at least the click event. If you're going to do all that work, why bother with this plugin?

8. A table and divs. Enclosed and in parallel. (Careful wiping that green sludge off your monitor; that's your brain that just exploded.)

Menus are "important" enough that HTML 5 has its own set of elements dedicated just to marking up menus. However, sensible, reasonably semantic standard patterns for use with HTML 4 and XHTML 1.x have evolved that address the issues I've mentioned (among many others). The markup for the example menu above could have been written as:

  <div class="context-menu-container" id="someId">
    <ul class="context-menu context-menu-theme-vista">
      <li class="context-menu-item">
        <span class="context-menu-item-inner">One</span>
      </li>
      <li class="context-menu-item">
        <span class="context-menu-item-inner">Two</span>
      </li>
      <li class="context-menu-item">
        <span class="context-menu-item-inner">Three</span>
      </li>
    </ul>
    <div class="context-menu-shadow"></div>
  </div>

One outermost div, with an id so your plugin doesn't get confused if you have multiple menus on a page, but one div so you can work with it as a single unit. An unordered list containing list items corresponding to the menu items, since that's the closest HTML4/XHTML 1 comes to HTML5's menu semantics. The Script that builds the thing can adapt the styles and attributes at the class+id CSS level, eliminating the need for hard-coded monsterpieces such as that style attribute for the table.

If you're going to write a jQuery plugin, live in jQuery. You can dynamically modify styles, attributes, content, even whole sections of how your page is rendered in the browser, without touching the basic markup. You can put the style bits that you intend to remain constant into a CSS file and link those styles to individual DOM fragments via classes and ids. That also keeps your HTML and CSS clean and cacheable.

If we don't write clean code, then having a lightning-fast browser on a petabit-Ethernet connection won't matter; users and reviewers will still complain that "your app is slow". Think before, as, and after you code, and remember:

First, do no harm.

'Least Knowledge' May Be More than You Think

I was chatting on IRC the other day and got into a discussion that stuck in the back of my mind until now. This guy was a fresh convert to the ideas of SOLID and to the Law of Demeter (or Principle of Least Knowledge). Now, these are principles I hold at least as strongly as the average experienced developer, and we eventually agreed to disagree over the propriety of code such as a line I just wrote (which in fact prompted this post).

Consider the Ruby code:

    data[:original_content] = article.content.lines.first.rstrip

This says:

1. "take whatever your article is;
2. get whatever its content attribute is or method returns;
3. (assuming that is a String or something that can be treated as one,) get its lines enumeration;
4. get the first entry in that enumeration;
5. (again, assuming it's a String or workalike,) strip any trailing whitespace from that string; and
6. stuff it into the collection data, indexed by the symbol :original_content."

It's not hard to argue that this is a bad code smell, being a long list of assumed return types and so on, except for one thing: in my view, the LoD does not cover sequencing a series of standard library calls.

Look at the code again. The bit that references my code is the fragment

  data[:original_content] = article.content

That fragment is clean; article is an object and content is an attribute of or method on that object. This code assumes that the value of article.content is a standard String. As a standard class, it's virtually guaranteed not to change in backwards-incompatible ways over the useful life of the code. What then? Standard library calls!

1. String#lines returns a standard Enumerator instance;
2. Enumerable#first (via the earlier Enumerator) returns a standard String instance again; and finally
3. String#rstrip cleans off any trailing whitespace.

Nowhere are used any internal APIs that could change. Breaking that one line apart into 5 would produce repetitive, non-semantic, anti-Ruby-best-practices code of the style pandemic on large Java projects. We're in Ruby (and Rails) for a reason: part of that reason is that the language idioms encourage an almost literary expressiveness that makes more explicit the reality that a program is a conversation between the humans developing and maintaining it, that just happens to be executable by a computer. Literate, semantic programming is like behaviour-driven development; once you've wrapped your head around it and see how much better a programmer it makes you, you really don't want to go back to BDUF jousting in Java or PHP.

On further review...

Now, if you argue that the LoD is an aid in reducing coupling between classes, which reduces per-statement complexity, I can see that. But, in the example above, we're back to the fact that there is only one dot before you start jamming together standard library calls. Had I instead written:

content = article.content lines = content.lines first_line = lines.first data[:original_content] = first_line.rstrip

then I'd argue that I've introduced three unnecessary local variables when only one (the second) could foreseeably change. If I later modified the return value from article.content to something other than a Stringlike object, then probably the assignment to lines would have to change. But only that one line — the same level of change that would be required in my current code.

Comments?