Delphi Daily WTF / Antipattern / Stupid code thread

[Pat Foley 51]

28 minutes ago, Uwe Raabe said:

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

 

Concede.

Edited August 11, 2021 by Pat Foley
crossing out

[Uwe Raabe 2057]

I'm probably going to regret it, but I'll throw this one into the discussion:

function BlankControl(EditControl: TObject; const EditMess: string; ShowErrorMsg: Boolean; MaxLen: Integer): Boolean;
begin
  Result := False;
  try
    if EditControl is TCustomEdit then
      Exit(length(trim((EditControl as TCustomEdit).Text)) < MaxLen);
    if EditControl is TComboBox then
      Exit(length(trim((EditControl as TComboBox).Text)) < MaxLen);
    if EditControl is TDBComboBox then
      Exit(length(trim((EditControl as TDBComboBox).Text)) < MaxLen);
    if EditControl is TDBRadioGroup then
      Exit((EditControl as TDBRadioGroup).Value = '');
    if EditControl is TDBLookupComboBox then
      Exit((EditControl as TDBLookupComboBox).Text = '');
    if EditControl is TDBCheckBox then
      Exit((EditControl as TDBCheckBox).State = cbGrayed);
  finally
    if Result then
      Showerror(EditControl, EditMess, false, ShowErrorMsg);
  end;
end;

 

[dummzeuch 1505]

1 hour ago, Uwe Raabe said:

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 

We could probably improve this with a try ... finally and exceptions. 😉

(I actually have some code that does this because a coworker complained loudly about the gotos I was originally using.)

 

Let's create a new thread with worse "improvements" ("Verschlimmbesserung") for this code.

[corneliusdavid 214]

I was surprised to check this thread this morning and see all the comments. I guess I should've mentioned I had already refactored this. Since I despise both GOTOs and EXITs, I rewrote it without any of them:

function BlankControl(EditControl: TObject; const EditMess: string; ShowErrorMsg: Boolean; MaxLen: Integer): Boolean;
begin
  // assume False, prove otherwise
  Result := false;

  if EditControl is TCustomEdit then
    Result := length(trim((EditControl as TCustomEdit).Text)) < MaxLen
  else if EditControl is TComboBox then
    Result := length(trim((EditControl as TComboBox).Text)) < MaxLen
  else if EditControl is TDBComboBox then
    Result := length(trim((EditControl as TDBComboBox).Text)) < MaxLen
  else if EditControl is TDBRadioGroup then
    Result := length((EditControl as TDBRadioGroup).Value) = 0
  else if EditControl is TDBLookupComboBox then
    Result := length((EditControl as TDBLookupComboBox).Text) = 0
  else if EditControl is TDBCheckBox then
    Result := (EditControl as TDBCheckBox).State = cbGrayed
  else
    KSMessage('control is not edit checked kslib', mtError, 0);

  if Result then
    Showerror(EditControl, EditMess, false, ShowErrorMsg);
end;

I guess I had also left out the final "else" clause in my original post--sorry about that, it's included here.

[Stefan Glienke 2002]

9 minutes ago, corneliusdavid said:

I had already refactored

And you indeed refactored it into the only sane form I can think of! Might have moved the Result := False into that final else though because it's only needed then.

And could have replaced the softcast with a hardcast because you already did the is check.

Edited August 11, 2021 by Stefan Glienke

[corneliusdavid 214]

7 minutes ago, Stefan Glienke said:

Might have moved the Result := False into that final else though because it's only needed then.

And could have replaced the softcast with a hardcast because you already did the is check.

True!

[Bill Meyer 337]

2 hours ago, dummzeuch said:

(I actually have some code that does this because a coworker complained loudly about the gotos I was originally using.)

Refer them to Donald Knuth, who carefully and clearly makes the case for using goto in some cases.

[Stefan Glienke 2002]

If you have a bad compiler that puts cold code into the middle of your hot loop where performance really matters instead of restructuring the code you need goto.

[Mike Torrettinni 198]

On 8/10/2021 at 5:54 AM, Tommi Prami said:

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.

I did find this in some of the open source projects, I guess nobody complained, yet.

[Tommi Prami 130]

On 8/10/2021 at 4:55 PM, Mike Torrettinni said:

 

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;

 

I'd do it some other way, ad begin end and comment in between or something that it is more clear.

 

(Thee was better version by earlier, so skip this 🙂 )

 

Edited August 12, 2021 by Tommi Prami
Added comment

[Sonjli 6]

The marvel:

if ThereIsAnError then
begin
  ShowMessage('This is the error');
  Abort;
end;

All exceptions in a big project with this simple, magic trick...

I had to work 2 weeks to change all the exception handling system.

[David Schwartz 426]

What's the problem with just saying: Exit(False) instead of using a goto to say: Result := False; Exit;

[Stefan Glienke 2002]

36 minutes ago, David Schwartz said:

What's the problem with just saying: Exit(False) instead of using a goto to say: Result := False; Exit;

Code that originated in a Version before 2009

[DPStano 15]

the worst I saw was 2,5k LOC to make a deep copy of the Form instead of creating new instance. 

Edited August 12, 2021 by DPStano

[Uwe Raabe 2057]

"Use constants" the boss told us:

const
   cDecimals0 = 0;
   cDecimals1 = 1;
   cDecimals2 = 2;
   cDecimals3 = 3;
   cDecimals4 = 4;
   cDecimals5 = 5;
   cDecimals6 = 6;
   cDecimals7 = 7;
   cDecimals8 = 8;
   cDecimals9 = 9;
   cDecimals10 = 10;
   cDigits0 = 0;
   cDigits1 = 1;
   cDigits2 = 2;
   cDigits3 = 3;
   cDigits4 = 4;
   cDigits5 = 5;
   cDigits6 = 6;
   cDigits7 = 7;
   cDigits8 = 8;
   cDigits9 = 9;
   cDigits10 = 10;
   cDigits11 = 11;
   cDigits12 = 12;
   cDigits13 = 13;
   cDigits14 = 14;
   cDigits15 = 15;
   cDigits16 = 16;
   cDigits17 = 17;
   cDigits18 = 18;
   cDigits19 = 19;
   cDigits20 = 20;
   cDigits21 = 21;
   cDigits22 = 22;
   cDigits23 = 23;
   cDigits24 = 24;
   cDigits25 = 25;
   cDigits26 = 26;
   cDigits27 = 27;
   cDigits28 = 28;
   cDigits29 = 29;
   cDigits30 = 30;
   cDigits31 = 31;
   cDigits32 = 32;
   cDigits33 = 33;
   cDigits34 = 34;
   cDigits35 = 35;
   cDigits36 = 36;
   cDigits37 = 37;
   cDigits38 = 38;
   cDigits39 = 39;
   cDigits40 = 40;
   cEndPos0 = 0;
   cEndPos1 = 1;
   cEndPos2 = 2;
   cEndPos3 = 3;
   cEndPos4 = 4;
   cEndPos5 = 5;
   cEndPos6 = 6;
   cEndPos7 = 7;
   cEndPos8 = 8;
   cEndPos9 = 9;
 <snip...>
   cEndPos997 = 997;
   cEndPos998 = 998;
   cEndPos999 = 999;
   cEndPos1000 = 1000;
   cEndPos9000 = 9000;
   cIndex0 = 0;
   cIndex1 = 1;
   cIndex2 = 2;
   cIndex3 = 3;
   cIndex4 = 4;
   cIndex5 = 5;
   cIndex6 = 6;
   cIndex7 = 7;
   cIndex8 = 8;
   cIndex9 = 9;
   cIndex10 = 10;
   cIndex11 = 11;
   cIndex12 = 12;
   cLength1 = 1;
   cLength2 = 2;
   cLength3 = 3;
   cLength4 = 4;
   cLength5 = 5;
   cLength6 = 6;
   cLength7 = 7;
   cLength8 = 8;
   cLength9 = 9;
   cLength10 = 10;
   cLength11 = 11;
   cLength12 = 12;
   cLength13 = 13;
   cLength14 = 14;
   cLength15 = 15;
   cLength50 = 50;
   cLength102 = 102;
   cLength117 = 117;
   cLength298 = 298;
   cLength618 = 618;
   cLength800 = 800;
   cStartPos0 = 0;
   cStartPos1 = 1;
   cStartPos2 = 2;
   cStartPos3 = 3;
   cStartPos4 = 4;
   cStartPos5 = 5;
   cStartPos6 = 6;
   cStartPos7 = 7;
   cStartPos8 = 8;
   cStartPos9 = 9;
<snip...>
   cStartPos995 = 995;
   cStartPos996 = 996;
   cStartPos997 = 997;
   cStartPos998 = 998;
   cStartPos999 = 999;
   cStartPos1000 = 1000;

 

[Guest]

7 minutes ago, Uwe Raabe said:

"Use constants" the boss told us:

const
   cDecimals0 = 0;
   cDecimals1 = 1;
   cDecimals2 = 2;
   cDecimals3 = 3;
   cStartPos9 = 9;
.................................................................
<snip...>
   cStartPos995 = 995;
   cStartPos996 = 996;
   cStartPos997 = 997;
   cStartPos998 = 998;
   cStartPos999 = 999;
   cStartPos1000 = 1000;

 

Edited August 12, 2021 by Guest

[Pat Foley 51]

4 hours ago, Sonjli said:

The marvel:

if ThereIsAnError then
begin
  //ShowMessage('This is the error');
  //Abort;
  halt; 
end;

All exceptions in a big project with this simple, magic trick...

I had to work 2 weeks to change all the exception handling system.

Upgraded to D14.4.2  

 

For EU End User input errors should provide helpful feedback. 'not enough data entry', 'too much data'

masked tedits help 

 

Exceptions need to be surfaced!   

 

[Tommi Prami 130]

On 8/12/2021 at 4:05 AM, Mike Torrettinni said:

I did find this in some of the open source projects, I guess nobody complained, yet.

We got hit by date/time string literals in the code.  Someone was using different locale and app crashed and burned 🙂

[David Schwartz 426]

20 hours ago, Stefan Glienke said:

Code that originated in a Version before 2009

Ahh, I didn't notice that. But ... I've seen stuff like that in newer code. Maybe it just never got refactored.

Edited August 13, 2021 by David Schwartz

[Fr0sT.Brutal 900]

9 hours ago, Tommi Prami said:

We got hit by date/time string literals in the code.  Someone was using different locale and app crashed and burned

Once I stumbled upon system ANSI encoding discussion and checked my apps with another ACP set in OS. That was a refreshing experience.