James's Weblog

All too often I see web sites open new browser windows using HTML and JavaScript code such as:

<a href="#" onClick="window.open('foo.html');"> <!-- BAD! -->

or:

<a href="javascript:window.open('foo.html');"> <!-- BAD! -->

This is one of my biggest peeves. Here’s why:

• For the first case, when users click on the link, a new window will appear, but the original window will scroll to the top of the page, causing the users to lose their place. In many cases this defeats the point of opening a new window at all.
• The first case prevents the browser from marking visited links properly. This compounds the problems of my first point.
• Both cases make the link completely useless in browsers that don’t have JavaScript enabled.
• Both cases prevent users from right-clicking on the link and explicitly requesting a new window. Instead of a new window going to the desired page, they’ll get a new window with the original page (in the first case) or with a blank page (in the second case).

Better:

<a href="foo.html" onClick="window.open('foo.html');return false;">

This solves all of the problems above. The “return false” is important! When the onClick event handler returns false, a browser does not follow the href link.

Better yet:

Re-evaluate why you want to open links in new windows at all. Most browsers have an “Open in New Window” command; if users want new windows, let them ask for it.

Sometimes if I don’t care about a C function’s return value, I explicitly discard it with a (void) cast:

(void) foo();

Why bother? After all, isn’t the return value implicitly discarded simply by not assigning it to anything?

A (void) cast is a comment to the reader stating, “Yes, the author is aware that this function returns something, has considered the consequences of ignoring possible error values, and just doesn’t care.”

Without the cast, the code says, “Did the author intentionally ignore the return value of this function call? Is there some failure case the author forgot to handle? Can something go wrong here?”

Furthermore, if something does go wrong, the (void) cast marks a possible failure point.

Yesterday I wrote about why output parameters should be carefully documented. If an error occurs, the output values should be documented as:

• being left in an undefined state
• being set to a known, bogus value (e.g. NULL, 0, −1, 0xFFFFFFFF, …)
• being left untouched

Which should you use?

Undefined state. Specifying that the output value is in undefined is certainly the easiest, laziest approach. It requires no additional effort for the function author, and if callers make assumptions about the output state, they take all the blame. Unfortunately, the lazy approach encourages future laziness. Especially if the function allocates resources, it becomes all too easy for a sloppy maintenance programmer to add an early exit-point and to create a resource leak.

Specifying that the output value be in a defined state can make a maintenance programmer more mindful. The function contract is not solely to benefit programmers writing callers; it also acts as a guide to anyone modifying the callee.

So if the output value is put in a defined state, what should that state be?

(more…)

People writing C code often write functions that look like:

/** foo -- Some function.
  *
  * PARAMETERS:
  *   someInput       : Some input value.
  *   OUT someOutputP : Set to some output value.
  *
  * RETURNS:
  *   Some error code.
  */
err_t foo(int someInput, data_t* someOutputP)
{
    err_t err = ERR_NONE;
    /* ... Code goes here ... */
    return err;
}

This isn’t good enough. Callers have no idea what to expect if an error occurs. Is someOutputP:

• in some totally undefined state?
• set to a known, bogus value?
• left untouched?

This is particularly important if the function allocates some resources in someOutputP and the caller needs to clean it up.

Whichever method you choose, you should choose one and document it. It needs to be part of the function’s contract. If left undocumented, callers can make assumptions about what state someOutputP is in, which is dangerous.

Worse, if you don’t specify the behavior, someone who modifies the code later might not know what the behavior is supposed to be and is likely to screw it up.

Better:

/** foo -- Some function.
  *
  * PARAMETERS:
  *   someInput       : Some input value.
  *   OUT someOutputP : On success, set to some output value.
  *                     On error, left untouched.
  *
  * RETURNS:
  *   Some error code.
  */
err_t foo(int someInput, data_t* someOutputP)
{
    err_t err = ERR_NONE;
    /* ... Code goes here ... */
    return err;
}

I’ll write my thoughts about the three behaviors later.

I’ve recently spent some time looking at the source code to several open-source projects.

I am appalled.

Here’s a sampling of what I’ve encountered:

• Buffer overflows. I cannot understand how C programmers continue to use gets, strcpy, and strcat when fgets, strncpy, and strncat are available. This isn’t that hard. (The C standard library shares a lot of blame; it never should have provided the unchecked versions, and it at least should have given gets/fgets and strcpy/strncpy consistent behaviors. strncpy and strncat aren’t as easy to use as they ought to be, either.)
• Unchecked errors. Writing error-checking code is hard and a lot of work, so apparently some people decide to forgo it altogether. Woe upon the user.

• Illegal language usage. Apparently a lot of people think it’s perfectly legal in C or C++ to name identifiers with a leading underscore. Often it’s not. Although in practice a namespace clash is unlikely, identifiers with leading underscores can intrude into the compiler’s namespace.

  I’ve also argued (unsuccessfully) with some people who recklessly invoke undefined behavior.

• Unmaintainable code. I suppose that I naively hoped that How to Write Unmaintainable Code was a work of fiction, but yes, Virginia, there are magic numbers. It’s odd that so much open-source code has poor documentation, sparse comments, and cryptic function and variable names. What’s the point of publishing your code if it’s unreadable?
• Inconsistent styles. More readability and maintenance problems stem from projects that lack strong core leadership.

What’s the big deal? The obvious problems are software vulnerabilities and broken software. I think that there might be a deeper problem, however.

Code begets code. Many people learn how to program by looking through other people’s code, and publishing bad code makes it too easy to propagate bad habits and to produce lousy de facto standards. Worse, people might copy-and-paste bad code outright. Yes, it can work the other way too; good code can be a paradigm for others to follow. Unfortunately, the sheer quantity of bad code available makes good code a needle in a haystack. Saturating the planet with immature software projects is not a step forward.

As potential pedagogues, open-source developers ought to be held to a higher standard. Unfortunately, reality offers a contrary opinion. A recent study indicates that email is more likely to be ignored when there are more recipients; are similar factors at play in the programming world? Does the nature of open-source code—where there’s a lack of accountability and where anyone can look at and change the source code—encourage its developers to shirk their responsibilities in the hopes that someone else will clean up their mess? Or are programmers in general just too lazy, too unskilled, too ignorant, or too apathetic? (I’m admittedly not a very experienced programmer; I lack design experience in particular. If nothing else, though, I’m meticulous (so Jeff Wong says) and consistent, and I make a conscious effort to be responsible with what I write!)

(Before anyone gets the wrong idea, I am not an opponent of open-source software. I’m picking on open-source code mostly because closed-source code isn’t available to be criticized.)