How Not to Write Code, II

Ok here's my view on the example in the last post.

It's a little hard to assign blame or point to a particular failure as the root cause of the problem. There are systemic problems beyond the obvious ones. I'll just point out a few problems.

First and foremost is using local time. Time values should always be handled as UTC values. In general, you only need local time if you are printing something for a human to look at. At this point, you can convert the timestamp from UTC to local time, if you know what time zone you are in. Conversely, you should convert from local time to UTC as soon as you read a local time.

You can in theory handle everything as a local time if you retain the time zone, but this violates an abstraction. You want to represent an instant in time, not the local convention for naming it.

The underlying problem is conflating the presentation with the representation. An instant in time is what you want to represent. You present that to the user as a series of characters. You don't want to represent the time as the character string you present to the user. (Consider, you may present the time to the user as a bitmap that shows a drawing of a calendar with the date circled in red. You wouldn't dream of using that bitmap as the internal representation!)

But the programmer who coded this up in the first place can't be blamed too much because there are problems with the software involved in presentation and persistence. The display is a web browser, and the current technology of page layout is to put markers around text. You cannot simply put a date object on a web page. You must create a text string that represents the date and use that instead. When a date is entered as part of a form, the character string entered is returned. The browser encourages sloppy practice.

The database was a major problem. The programmer went out of his way to bypass the database schema by declaring everything as an unstructured list of strings. Now this in itself is not necessarily a bad thing, but to do this correctly you want to add an abstraction layer that manages the mapping from objects to their serialized forms. Part of the purpose of that abstraction layer is to hide the underlying persistent representation. The user shouldn't be aware that there are strings at the bottom. The mapping layer ought to provide a reasonable set of primitive types (booleans, integers, enums) in addition to a few of the more complex types one might store in a database, like dates and URLs. The mapping layer also ought to canonicalize and validate the data. If this existed, the programmer would most likely have used the abstraction layer to store the date objects rather than serializing them to strings by hand. It would be less likely that local times would have been stored in the database.

Ideally, you'd like a rich object model that worked from end-to-end. Requests to the server would contain embedded date objects, these date objects could be written to the database directly. When presenting data, the date objects would be retrieved from the database and placed in the appropriate data structures to send back to the browser. Unfortunately, neither the viewing nor the persistence layers provide this.

The last issue is the insane workaround. The programmer was obviously clever, but what on earth possessed him to write that? How did that pass code review?! The problem is that there is corrupted data in the system. For heaven's sake don't add helper functions to make more of it!

There are dozens of ways to fix this bug, and nearly all of them are better. Here's one way (admittedly off the top of my head):

1. Add the ‘date’ abstraction to the database that keeps dates and times in UTC format and hides the mechanism of serialization.
2. Add new fields to the existing objects to replace the ones holding strings in local time. Initialize the new fields with the UTC dates and times that correspond to the local time strings.
3. Deprecate the old field. Write code that throws an exception if any other code tries to read or write it.

You'd also have to fix the remaining code to work correctly. The existing code misuses Date objects, so they are definitely suspect. A trick here would be to make some scaffolding code. Define a new class (call it UTCDate or something), that is a simple shim over the real date class. It should have the exact same API as the Date class, except it should not have the constructors or selectors that caused the original problems. Make the database return objects of this class. Do not allow coercion between Date and UTCDate. Now go through the code and fix the type errors. You'll end up following the data flow of Date objects through the code. Since Date and UTCDate are virtually the same, most of the time you'll just have to replace one with the other. The places where you can't are the problem spots.

Once everything works with UTCDates, you can textually replace UTCDate with Date (provided you made the API compatible) and toss out the scaffolding.