CodeSOD: Join Us Again

Submitter "Anonymouse Lee" inherited a Pascal program that started its development back in 2002. It's seen some rough handling over the years, and that alone has bred some WTFs, but this particular block was created by the founder of the company, who bills themself as a "highly technical founder".

The goal is this: the program needs to display a data grid, and depending on the recordType of each row in the grid, that row needs to be displayed slightly differently.

Let's take a look at the code:

var x,y : int; recordtypes : variant; begin recordtypes := sql('select key_value, code from recordType'); for x := 0 to grid.RecordCount - 1 do begin for y := 0 to hi(recordtypes,2) do begin if grid[x].key_value = recordtypes[0,y] then case recordtypes[1,y] of 'a': begin *do stuff for type a' here* break; end; 'b': begin *do stuff for type b' here* break; end; 'c': begin *do stuff for type c' here* break; end; end; end; end;end;

grid is a global variable containing the data grid to display. Here, we run a query to look up all the possible record types, then for each row in the grid, we iterate across each recordtypes entry, and compare the key_value fields. Then, if the key value matches, we do a case based on the code field.

It's a join. We've reimplemented a join on the client side. But honestly, we've made the join even worse. It's an awkward nested for loop with a case statement, even though for any given successful match on key_value, there's only one possible case branch that could be true.

There's a deeper WTF though, that highlights a common anti-pattern that I've seen in many places: we're tying code logic to values in database fields. The code fields can't change in the database. Adding a new row to the recordTypes table won't handle the special case of rendering that new row- you need to roll out code changes to support data changes. That's a hard problem to be able to route around, and the obvious solutions to it frequently lead you down the path of the inner platform. But at it's core, it's not a code or even a software architecture problem: it's an analysis problem and a design problem. The problem domain has been expressed in terms that require this kind of brittle code, and this kind of brittle code is so common that many developers don't even blink at it. Worse: there's no easy solution to this problem, and sometimes we just end up implementing the brittle code because actually solving the problem "correctly" is prohibitively difficult.

But that's all "big" stuff, all high level stuff, all "above our pay grade" stuff, and certainly it was above our submitter's pay grade, and since the code was already baked into this form, it was too late to make any massive changes.

Instead, Anonymouse just altered the query that populated grid, and ensured that grid had a code field, removing the need for the inner loop.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.