CodeSOD: Expiration Dates

Last week, we saw some possibly ancient Pascal code. Leilani sends us some more... modern Pascal to look at today.

This block of code comes from a developer who has... some quirks. For example, they have a very command-line oriented approach to design. This means that, even when making a GUI application, they want convenient keyboard shortcuts. So, to close a dialog, you hit "CTRL+C", because who would ever use that keyboard shortcut for any other function at all? There's no reason a GUI would use "CTRL+C" for anything but closing windows.

But that's not the WTF.

procedure TReminderService.DeactivateExternalusers;var sTmp: String;begin // Main Site if not dbcon.Connected then dbcon.Connect; if not trUpdate.Active then trUpdate.StartTransaction; qryUsersToDeactivate.Close; sTmp := DateTimeToStr(Now); sTmp := Copy(sTmp, 1, 10) + ' 00:00:00'; qryUsersToDeactivate.SQL.Text := 'Select ID, "NAME", ENABLED, STATUS, SITE, EXPIRATION '+ 'from EXTERNAL_USERS ' + 'where ENABLED=1 and EXPIRATION<:EXPIRED'; qryUsersToDeactivate.ParamByName('EXPIRED').AsDateTime := StrToDateTime(sTmp); qryUsersToDeactivate.Open; while not qryUsersToDeactivate.Eof do begin qryUsersToDeactivate.Edit; qryUsersToDeactivate.FieldByName('ENABLED').AsInteger := 0; qryUsersToDeactivate.Post; qryUsersToDeactivate.Next; end; if trUpdate.Active then trUpdate.Commit; // second Site// same code which does the same in another databaseend;

This code queries EXTERNAL_USERS to find all the ENABLED accounts which are past their EXPIRATION date. It then loops across each row in the resulting cursor, updates the ENABLED field to 0, and then Posts that change back to the database, which performs the appropriate UPDATE. So much of this code could be replaced with a much simpler, and faster: UPDATE EXTERNAL_USERS SET ENABLED = 0 WHERE ENABLED = 1 AND EXPIRATION < CURRENT_DATE.

But then we wouldn't have an excuse to do all sorts of string manipulation on dates to munge together the current date in a format which works for the database- except Leilani points out that the way this string munging actually happens means "that only works when the system uses the german date format." Looking at this code, I'm not entirely sure why that is, but I assume it's buried in those StrToDateTime/DateTimeToStr functions.

Given that they call qryUsersToDeactivate.Close at the top, this implies that they don't close it when they're done, which tells us that this opens a cursor and just leaves it open for some undefined amount of time. It's possible that the intended "close at the end" was just elided by the submitter, but the fact that it might be open at the top tells us that even if they do close it, they don't close it reliably enough to know that it's closed at the start.

And finally, for someone who likes to break the "copy text" keyboard shortcut, this code repeats itself. While the details have been elided by the submitter // same code which does the same in another database tells us all we need to know about what comes next.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!