Delphi Daily WTF / Antipattern / Stupid code thread

[Tommi Prami 130]

Would be interesting to see all kinds stupid code out there, possibly with some arguments/proof why some code is bad.

For sure there are tons of different coding styles, but still. Sometimes you stumble upon something that is "pure genius" or something that hides the problem that like David Copperfield. Or what are just scary, code that has been working for ages in production but should not 🙂

 

I'll post some examples into the reply

 

Hope someone all post their own and  biggest WTF of all times 😄

 

-Tee-

 

PS. Trey to stay civil and nice, there always are differences of opinion and some prefer the mother and other the daughter. If we can get this thread flöying there will be good information for many I am sure.

Edited August 10, 2021 by Tommi Prami
(More stuf)

[Tommi Prami 130]

if A > B then;
...

if A > B then
  A := B
else;

Semicolon right after the then or else is legal syntax, and can be used rightly, but I think those are possible point of errors and not easy to spot

[Tommi Prami 130]

try
  ...
finally
end;

Empty finally blocks, maybe  .free call is missing or after refactoring it has become obsolete but left behind. Not sure can the compiler optimize that out, that seems to me unneeded code that should be removed or fixed other way.

[Tommi Prami 130]

 IntToStr(LDataSet.AsInteger);

Could use AsString directly.

[Tommi Prami 130]

try
  ..
except
end;

eating all exceptions without commenting why exactly, and why it isn't or can't be something more specific.

[Tommi Prami 130]

StrToDate('2021/12/31');

Using StrToDate() etc with Date/Time literal in code, that most likely will fail in other locals than yours, Also the EncodeDate etc routines giving numbers is better IMHO.

[Tommi Prami 130]

with MainForm, ButtonCancel do
...

with MainForm do
  with ButtonCancel do
  ...

Nested with clauses are pure evil to refactor out. If it is complex code and there are common properties (Like Caption in VCL on above) how to decode which is which.

Made feature request of this one way way back. Hope they make refactorer that would cast the first stone, so can manually refactor rest. : 
https://quality.embarcadero.com/browse/RSP-18953

[aehimself 396]

My forever favorite stays this:

Function GetUserID(APageIndex: Integer): Integer;
Begin
 Case APageIndex Of
   0: Result := -1;
   1: Result := -1;
   2: Result := -1;
   3: Result := -1;
   [...]
   50: Result := -1;
   Else Result := -1;
End;

Probably it had functionality a long time ago, but this method was still called from a live part of the code when I found it...!

 

There were also some fancy fails (mainly memory leaks), including but not limited to:

TMyClass = Class(TObject)
private
 FOwner: TComponent;
public
 Constructor Create(AOwner: TComponent); ReIntroduce;
End;

Constructor TMyClass.Create(AOwner: TComponent);
Begin
 FOwner := AOwner;
End;

Or the other, which was probably a typo but took me a while to finally realize the issue:

TMyClass = Class(TComponent)
public
  Constructor Create(AOwner: TComponent); Override;
End;

Constructor TMyClass.Create(AOwner: TComponent);
Begin
 inherited Create(Owner);
End;

 

[dummzeuch 1505]

My all time favorite:

if SomeErrorCondition then
  Exception.Create('Errormessage goes here');

(raise is missing, just in case you didn't notice.)

 

Or alternatively:

raise exception('SomeErrorMessage');

(create is missing)

Makes for a "nice" Access Violation in the exception handler.

Both were in my own code, but the first one was also in some older RTL and JVCL and Indy code (all fixed now).

 

I blogged about this a while ago. (OMG, back then we still hat Google+ !)

Edited August 10, 2021 by dummzeuch

[mvanrijnen 123]

Leaving Delphi 7, and keep hoping on a reasonable release of D10 was the biggest mistake for most Delphi programmers i think. 🙂

 

 

[Fr0sT.Brutal 900]

try
  ...
except on E: Exception do
  ...
  raise E;
end

Gives scary and mystic AV somewhat later after the problematic line in OS callstack. Real PITA to find and fix.

[luebbe 26]

3 hours ago, Tommi Prami said:

if A > B then;
...

if A > B then
  A := B
else;

Semicolon right after the then or else is legal syntax, and can be used rightly, but I think those are possible point of errors and not easy to spot

for I := low to high do;
begin
  ...
end;

falls into the same category.

[Mike Torrettinni 198]

11 minutes ago, luebbe said:

for I := low to high do;
begin
  ...
end;

falls into the same category.

 

Could this fall into the same category, or is this valid case?:

 

in Delphi 10.2.3  Vcl.ActnMenus:

procedure TCustomActionMenuBar.ProcessMessages;
var
  Msg: TMsg;
begin
  while ProcessMessage(Msg) do;
end;

 

[Uwe Raabe 2057]

As ProcessMessage already does all the work, there is not much to do in the body of this loop.

[Stefan Glienke 2002]

1 hour ago, Mike Torrettinni said:

Could this fall into the same category, or is this valid case?:

 

procedure TCustomActionMenuBar.ProcessMessages;
var
  Msg: TMsg;
begin
  while ProcessMessage(Msg) do;
end;

 

No, it's just shorter than writing:

 

var
  LKeepProcessing: Boolean;
begin
  repeat
    LKeepProcessing := ProcessMessage(Msg);
  until not LKeepProcessing;
end;

 

[David Schwartz 426]

Many years ago I found some Delphi code in units I was working with that looked odd. I stared at it for quite a while and finally realized the problem. It was written by a guy who had been working in MUMPS for a long time and Delphi was relatively new to him. 

 

In MUMPS everything is an array, except a string is a string, whereas in Delphi a string is an array of characters. In MUMPS, a "string" variable is more like a TStringlist in Delphi.  

 

In Delphi, the first character of a "string" returned by MUMPS would be accessed in MUMPS like this: firstChar := Copy( aStrList[0], 1, 1 );

There's a function in MUMPS called 'piece' that's equivalent to Delphi's Copy function, and it's used everywhere, often nested 2 or 3 levels deep in the same line of code just to extract a character or substring from something.

 

In this case, a call being made to a service returned something that was put into a (Delphi) string. The code referred to theReturnValue[1] which is perfectly valid in Delphi, only it refers to the first character of the string. In MUMPS it's analogous to a TStringlist and refers to the first string [aka, piece(theReturnValue,1)] that was returned. Sometimes it would be empty (ie., '') and would throw an exception. Unfortunately, because of the way exceptions were handled, all we got was a notification that something somewhere tried to read memory at location 0.

 

Another problem in this same code was like this:

aReturnValue := theCallStatus;  -- both of which were integers.

 

But then it did this:

if aReturnValue <> '0' then ...

 

Several of these calls returned a value of zero (0) on success, and several returned a value of one (1) on success and zero on failure.

 

SIDE NOTE: the devs here paid no attention to Hints and Warnings from the compiler, so there were literally hundreds when we did a build. I'm sure there were some that related to these code constructs, but they were all ignored.

 

In this case, the code was checking for a failure, and was equivalent to: if aReturnValue <> $60 then ... 

 

The return on success was one (1) so most of the time the logic worked and was consistent with what you'd think by reading the code -- it wasn't $60 either. But sometimes it would return a zero and the test failed to notice that the call itself failed.

 

The purpose of the test was to determine if the returned data was valid or not, and the test failed to recognize that the call failed, and it was processed normally, ultimately returning a success code to the caller in virtually every case, even when there were errors.

 

The first thing it did in many cases was try to access something from an empty TStringlist, which threw a List Index Out Of Bounds error. Again, because of the way exceptions were being handled, all we knew was something somewhere threw an LIOOB error, but not where. (Besides, it was a long ways from where the TStringlist was created.)

 

Since the code returned a success flag (because the failure wasn't detected), something way downstream would eventually throw another exception, usually when it tried to use some random data it thought was valid, as an index into a database, which threw a database exception that was impossible to trace back to the code shown above.

 

This resulted in a steady stream of intermittent errors that could never be reproduced or diagnosed. The QA Dept concluded they were just spurious user errors on the part of the customer.

 

I don't know what these kinds of mental translation errors are called, but in normal human parlance they'd probably be called a faux paus. I found a total of 7 distinct forms of these all used in conjunction with the same basic interface call. 

 

The programmer used a "copy-and-paste" style of programming, so he'd copy 20-30 lines of code from one place and paste it into the code where he needed it, faux paus and all. I found over 100 places where he pasted this code, and there was an average of 4 of these faux paus errors in each one.

 

I fixed about 20 of them in the most used parts of the code, and the performance of the entire system improved enough that several people noticed it. Also, the spurious errors disappeared.

 

(I was actually told to revert the changes because at this place, Development was not allowed to submit bug tickets. Only QA and the customer could do that. The errors from these bugs were data-dependent and impossible to reproduce, so they were never documented properly enough to warrant being fixed.)

 

[Stefan Glienke 2002]

Here is some code that I recently wrote that looks stupid but indeed is clever because the compiler is stupid:

 

if P = nil then
  Exit(Boolean(P));

P is of type Pointer and it ensured that it's in the same register that the result is being returned so in case it's nil there is nothing to do (if you would write Exit(False) the compiler would insert extra instruction to actually make the register zero (which it already is)

[corneliusdavid 214]

I recently inherited an old project that had many bad code examples that looked like whenever anyone made a change to the code, they just left it like they found it instead of refactoring. There were empty IF-blocks, while-true loops, exit statements one line before the end of a procedure, and one of my favorites:

for j := 0 to 0 do begin
    Temp := StartTop[j] + CompRow;
    CreateLabel(CNameL[j], LeftSide - 4, Temp, 150);
   ...

All of these were in that ONE project. But the worst code in that I found in that project was a bunch of GOTOs:

function BlankControl(EditControl: TObject; const EditMess: string; ShowErrorMsg: Boolean; MaxLen: Integer): Boolean;
label
  900, 1000;
begin
  if EditControl is TCustomEdit then
    if length(trim((EditControl as TCustomEdit).Text)) < MaxLen then
      goto 1000
    else
      goto 900;

  if EditControl is TComboBox then
    if length(trim((EditControl as TComboBox).Text)) < MaxLen then
      goto 1000
    else
      goto 900;

  if EditControl is TDBComboBox then
    if length(trim((EditControl as TDBComboBox).Text)) < MaxLen then
      goto 1000
    else
      goto 900;

  if EditControl is TDBRadioGroup then
    if ((EditControl as TDBRadioGroup).Value = '') then
      goto 1000
    else
      goto 900;

  if EditControl is TDBLookupComboBox then
    if ((EditControl as TDBLookupComboBox).Text = '') then
      goto 1000
    else
      goto 900;

  if EditControl is TDBCheckBox then
    if ((EditControl as TDBCheckBox).State = cbGrayed) then
      goto 1000
    else
      goto 900;

900:
  Result := false;
  exit;
1000:
  Result := true;
  Showerror(EditControl, EditMess, false, ShowErrorMsg);
end;

 

[Stefan Glienke 2002]

Apart from the names of the labels its actually not that terrible tbh - certainly better than repeating the same code over and over in every check.

[Fr0sT.Brutal 900]

2 hours ago, Stefan Glienke said:

Apart from the names of the labels its actually not that terrible tbh - certainly better than repeating the same code over and over in every check.

The code is pretty clear (surprise) but could be refactored to a single big "if" making goto's unnecessary

if ( ((EditControl is TCustomEdit) and (length(trim((EditControl as TCustomEdit).Text)) < MaxLen)) or

...

) then ...

 

Probably the author wasn't aware of short boolean eval so he didn't use AND operator.

Edited August 11, 2021 by Fr0sT.Brutal

[Stefan Glienke 2002]

3 minutes ago, Fr0sT.Brutal said:

The code is pretty clear (surprise) but could be refactored to a single big "if" making goto's unnecessary

Certainly but that would change the behavior - currently if the class is found it exits, regardless. With a giant if and/or statement it would continue to evaluate the is checks.

[dummzeuch 1505]

+1 for goto! 😉

[Fr0sT.Brutal 900]

3 hours ago, Stefan Glienke said:

Certainly but that would change the behavior - currently if the class is found it exits, regardless. With a giant if and/or statement it would continue to evaluate the is checks.

Insignificant difference IMHO

[Pat Foley 51]

3 hours ago, Stefan Glienke said:

Certainly but that would change the behavior - currently if the class is found it exits, regardless. With a giant if and/or statement it would continue to evaluate the is checks.

 

 

 

// lost editor code box somehow sorry.

Fix up at top Result := False; once 

 

function BlankControl(EditControl: TObject; const EditMess: string; ShowErrorMsg: Boolean; MaxLen: Integer): Boolean;
//fall back label gotoTrue spare
  
  procedure gotoTrue;
       begin 
           Result := True;
         Exit;
       end;
begin
  Result := false;

  if EditControl is TCustomEdit then
    if length(trim((EditControl as TCustomEdit).Text)) < MaxLen then
      gotoTrue;
  if EditControl is TComboBox then
    if length(trim((EditControl as TComboBox).Text)) < MaxLen then
      gotoTrue;

  if EditControl is TDBComboBox then
    if length(trim((EditControl as TDBComboBox).Text)) < MaxLen then
      gotoTrue;

  if EditControl is TDBRadioGroup then
    if ((EditControl as TDBRadioGroup).Value = '') then
      gotoTrue;

  if EditControl is TDBLookupComboBox then
    if ((EditControl as TDBLookupComboBox).Text = '') then
      gotoTrue;

  if EditControl is TDBCheckBox then
    if ((EditControl as TDBCheckBox).State = cbGrayed) then
      gotoTrue;
      
//gotoTrue: 
//result := True;      
end;

...

if BlankControl(EditControl, EditMess, false, ShowErrorMsg,0) then
  Showerror(EditControl, EditMess, false, ShowErrorMsg);// Move the spaghetti hidden in the goto sauce 

[Uwe Raabe 2057]

Ehm! That Exit call will only exit procedure gotoTrue, but not function BlankControl.

 

Funny, that such a suggested improvement perfectly qualifies for the reason of this thread 

Edited August 11, 2021 by Uwe Raabe