Bad Habits : Abusing bit columns
See the full index.
I review a lot of DDL code from various teams, and I come across this often:
ALTER TABLE dbo.Widgets ADD IsNotCertified bit NULL;
When I see a bit column allowing NULLs, I always ask:
Is the intention here to have 3-valued logic?
Does NULL mean something different from 0, or will all future code use COALESCE to make them the same?
In some cases, NULL and 0 might have distinct meanings. For example, a bit column could represent whether a setting is on or off; NULL could mean the setting hasn't been configured yet, we haven't checked it yet, or it isn't valid for this entity. All of which could be important facts to distinguish in our business logic – though I would probably argue for a
tinyint column like
ActiveState that contains on (1), off (0), and lookup values for other state possibilities (more info here).
More often than not, though, it turns out that the developer was afraid to add a NOT NULL column to en existing table because, on ancient versions of SQL Server, that would always block the entire table. (Okay, maybe that is a concern on your version/edition, but then I would expect the code to include batching updates to set the values to 0, then add the NOT NULL constraint.)
And in this case, when the bit column is defined "backward," I also ask:
Do we really want to express this so that 1 means no and 0 means yes?
Because, inevitably, something a lot more bizarre happens with a column called
IsNot<something>. Recently I was reviewing a horribly performing multi-statement table-valued function, and it had this code:
/* added by M.C. Escher */ INSERT @Awful SELECT WidgetID, ~COALESCE(IsNotCertified, 0) FROM dbo.Widgets WHERE ...
At first I thought, surely, DPA injected some artifact during collection or display, or a typo somehow made it into the code. But then I reviewed the table defined at the beginning and, sure enough, this was intentional:
RETURNS @Awful table ( WidgetID int, IsCertified bit, ...
The column here has the opposite meaning of the column in the table, so we have to flip the bit (and also deal with the fact that NULLs are possible but don't mean anything different). This combination led to the use of BITWISE NOT, which I'm sure the author thought was clever, but gave no consideration whatsoever to every future reader that looks upon this code and could suddenly look like an Edvard Munch subject.
But hey, at least it wasn't this bad:
WHERE NOT (~COALESCE(IsNotCertified, 0) <> -1)
Personally, I would go to great lengths to not make the code so cryptic to save a few characters – especially shorthand using BITWISE operators, which I've whined about before. The following is wordier but a lot more self-documenting in terms of intent, and checks the column that can't have two different values to express the same meaning so we don't have to worry about coalescing those:
INSERT @Awful(WidgetID, IsCertified) /* including the column list helps */ SELECT WidgetID, IsCertified = CASE WHEN IsNotCertified = 1 THEN 0 ELSE 1 END /* ^ an alias here matching the column name can't hurt */ /* a comment here explaining why we're flipping can't hurt, either */ FROM dbo.Widgets WHERE ...
However, this is still Escher-esque to me, in that it makes me do mental gymnastics. Where possible, I would either name the column in the return table to match the source, or go back to the design of the table and flip the column itself to
IsCertified NOT NULL DEFAULT 0 – so 1 means 1, 0 means 0, and that's all there is to it.
Defining something backward on purpose seems, well, backward.
See the full index.
This reminds me of a column we have…it's not as obvious, but it still took some time to get used to called "IsClosed", so 1 means closed and 0 means open. I suppose this one could go either way, but I feel like IsOpen and 1 meaning open feels more "normal", especially in this case since "Open" is the default state. That said, we've got plenty of "DisableFooBar" fields…yuck.