David Millington of Parnassus had a fun little Bad Delphi Code contest and just announced the results. My submission got an honorable mention. (Nothing like getting an honorable mention in bad code writing to give people confidence in my software. “Hurry and buy today!” 🙂 )
Seriously, it was a lot of fun. Apparently, results were judged on creativity, deviousness, and backstory, to submit “the worst believable code snippet or small app that you can [write], in the spirit of amusing, entertaining, or horrifying the reader.”
All of the submissions were entertaining, but I especially liked the Break entry. Apparently, Break and Continue are not reserved words in Delphi, and are pseudo-implemented in the System unit. It is easy to define your own Break and Continue functions and, because of scoping rules, cause your Break/Continue command to be called instead of the regular Delphi command in subsequent code, breaking everyone’s nice loop structures 🙂
To me, this one exemplifies truly “evil” (not just bad) code, because it betrays user’s expectations. Bad code is easy to write and, admit it, all of us have done it. Under the pressure of deadlines or just plain laziness (or sometimes incompetence), we have written long, muddled, and just plain wrong code that is a nightmare to debug and maintain. However, if it does work, people may never even see it or have to think about it. But when you betray user’s expectations, that is true evil code. Component writers must guard most closely against this type of code. Because not only do we break our code, we break our customers.
For example, one I encountered recently is in the FMX TControl3D code. In FMX, there is a TControl.Position property. To have 2 controls in the same position, you can easily assign to the position property:
Label1.Position := Button1.Position;
Everything we have learned with the VCL and Delphi style guides condition us to expect that the button1’s position object is copied to Label1’s position property and everything just works.
In the FMX 3D library, there is also a TControl3D.Position property. However, it betrays our expectations. Here is the declaration of the Position property:
property Position: TPosition3D read FPosition write FPosition;
If we do code like:
Cube1.Position := Sphere1.Position
Instead of copying the TPosition3D object, it just sets the reference to the same TPosition3D (and throws away without freeing the old TPosition3D object). The code appears to work at first. However, not only have we leaked memory, we have tied the two objects’ positions together and changing one will move the other. “Luckily,” this code blows up on desktop (though it wouldn’t with ARC code) when we try to free both objects as they both try to free the same TPosition3D object.
This is truly evil and incompetent code. We have led our users astray and made their code bad through our own incompetence.
My Bad Delphi Code Submission
But enough about that. I saw that David’s readers wanted access to the code for every submission. Here is my submission:
This code is inspired by Embarcadero's FMX TPathData.SetPathString method, which parses a path for you. In this code, they had a loop for getting the next command in the path (move, line, etc) where they would compare the current token against 18 commands, every single time even when there was a match found earlier: while TokenBuilder.Length > 0 do begin Token := TokenBuilder.Chars; TokenBuilder.Remove(0, 1); if Token.IsInArray(['z', 'Z']) then ClosePath; if Token = 'M' then begin […] end; if Token = 'm' then begin […] end; if Token = 'L' then begin […] end; […] Finally, in XE8, they fixed it and used a case statement. I thought I would "improve" on their code by doing the same thing but with longer strings and adding in unnecessary code for not finding the string. I made sure that the short circuit for the if statements didn't work as well (wrong order). procedure TForm1.StringCompareUnOpt(aString: String); var Found: Boolean; begin Found := False; if aString = 'A' then begin Log('B'); Found := True; end; if (aString = 'One') and (not Found) then begin Log('Two'); Found := True; end; if (aString = 'Foo') and (not Found) then begin Log('Bar'); Found := True; end; if (aString = 'Four Score') and (not Found) then begin Log('And Seven Years Ago'); Found := True; end; if (aString = 'Hello') and (not Found) then begin Log('World'); Found := True; end; if (aString = 'Question') and (not Found) then begin Log('Answer'); Found := True; end; if not Found then Log('Unknown'); end; Now, to make it interesting, I decided to "optimize" the code by creating a constant array of hashes for each string and then comparing the current string's hash code to the constant array values. procedure TForm1.StringCompareOpt(aString: String); begin // "Optimized" string compare // Pros: // Shorter looking code (if you disregard setting up the constants) // "Looks" like it could be faster as case is just a integer comparison instead of string comparison // Cons: // Wrong (case insensitive vs original case sensitive compare), // Brittle (what if Strings const changes? Need to regenerate hashcodes. // what is GetHashCode implementation changes? Wrong answers) // Slower (GetHashCode can be slow) case aString.GetHashCode of StringA: Log('B'); StringOne: Log('Two'); StringFoo: Log('Bar'); StringFour: Log('And Seven Years Ago'); StringHello: Log('World'); StringQuestion: Log('Answer'); else Log('Unknown'); end; end; What I like about this code is that it *almost* looks brilliant. The code is much shorter and in theory using a case statement is faster. However, as mentioned in the code it is wrong sometimes, brittle, and slower as well as being a lot more work to set up. FTW! 🙂 Tom
The full code, including test program, can be downloaded here. Happy CodeSmithing!