Monday, July 02, 2007

Mired in quicksand

Okay, I lied. I didn't know it at the time, but it undeniably so.

I have never particularly liked delving into someone else's code. There's always that feeling of disorientation that comes from looking at a landscape that was apparently painted by the love child of Pablo Picasso and Salvador Dali while on heavy-duty psychedelic drugs, and that's when the code is fundamentally sound. Let's face it, at a certain level, code is more a matter of art than engineering, and each of us has our own aesthetic. My artistic sensibilities are easily offended, but I can usually make allowances for taste by gritting my teeth and muttering "chacun a son gout" under my breath.

The code I was going to "fix", though, is diseased to the root. Patternitis, and a fatal case of it, I'm afraid. I tried a simple Façadectomy, but I found that the unnecessary wrappers had metastacised throughout the entire body. Why do people insist on doing this:

function proprietary_do_something($param) {
 return generic_do_something($param);
 }

Now, that kind of thing might be forgivable if one had created, say, a façade interface for a database connection and a particular database's class happened to correspond exactly to the interface. There is no trace of that kind of foresight here. For database activity, one basically has a choice between mySQL. And there is neither interface nor class to be seen — nothing but proprietary wrappers around native PHP functions. I have to be generous here and try to convince myself that the wrappers had once been necessary, but the fact that the only difference between the proprietary method names and the built-in ones is a prefix leads me to believe otherwise.

Then there are the multiple if statements in a single function that repeatedly test exactly the same conditions. To the Notes folk out there, many of these instances are the equivalent of:

@If(
 @IsError(@DbLookup("":"NoCache"; ""; "view"; key; 2);
 "";
 @DbLookup("":"NoCache"; ""; "view"; key; 2))

Yep, they're not just doing the same test over and over, but they're doing the same high-cost test over and over. Now, I ain't no PHP guru, but I'd'a thunk that doing the test once and setting a flag variable based on the result, then basing your conditionals on the flag woulda been the way to go. But what do I know? The last time I did anything in PHP, Rasmus Lerdorf was still in Toronto and PHP stood for "Personal Home Page Tools".

So the code that I once thought not too bad (if one ignored the HTML) goes from being the backbone of my project to a mere sketch of the functionality that I'll need. I'll grant that it works, but merely working is not enough. This thing does "thumbnails" by setting the width and height attributes on full-sized images (when PHP can easily do image resizing/resampling — not fast enough to create them on the fly, but they can be created and stored when the big image is uploaded and modified when the application is reconfigured). Even the database schema needs help (said the Notes guy), and so I'll need to include a conversion utility with the "installer". I kinda feel like Mike Holmes (of Holmes on Homes, whose claim to fame is fixing criminally shoddy home renovation work).

I've got a lot more work to do, but at least I can take some pride in what I'm doing. Shouldn't we all?

3 comments:

Rob McDonagh said...

Hrm. At the risk of giving too much credit, is there any chance they were trying to insulate an API-like interface of their own from possible changes to the vendor's API? I've seen that done before, but only in a case where there were an awful lot of moving parts and most of them weren't under the dev's control. Oh, and the dev was paranoid about one particular thing.

The premise was that the vendor could change the names of their functions (and the parameters as well) without affecting most of the other moving parts. I thought it was juuuust a bit much (not to mention of questionable utility even granted the premise), but apparently it was a case of "twice burned forever shy" with this particular developer - after a couple of bona fide disasters, he insisted on doing this to every system he worked on.

Probably giving way too much credit, but as you know my philosophy on another person's code is always to assume there was a reason at the time. Not always a *good* reason, of course. :D

Stan Rogers said...

I tried that particular generousity tack, Rob, but after a while it just stopped making sense. It's sort of like wrapping LotusScript in LotusScript in order to isolate your code from changes to the LotusScript language.

In any case, the new verson will have a set of classes implementing a common interface, making db choice easier (mySQL is the wild card in this game -- there's no guarantee that it will remain free as in beer much longer, and ISPs may begin to drop it as a standard feature).

Karen said...

Nice redesign, btw. Green is good. :-)