Jump to content
Tommi Prami

Delphi Daily WTF / Antipattern / Stupid code thread

Recommended Posts

28 minutes ago, Uwe Raabe said:

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

 

Concede.

Edited by Pat Foley
crossing out

Share this post


Link to post

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;

 

Share this post


Link to post
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 :classic_biggrin:

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.

  • Like 1

Share this post


Link to post

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.

  • Confused 1

Share this post


Link to post
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 by Stefan Glienke
  • Like 1

Share this post


Link to post
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!

Share this post


Link to post
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.

Share this post


Link to post

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.

  • Like 2

Share this post


Link to post
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.

  • Like 1

Share this post


Link to post
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 by Tommi Prami
Added comment

Share this post


Link to post

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.

  • Sad 1

Share this post


Link to post
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

  • Like 1

Share this post


Link to post

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

Edited by DPStano
  • Like 1

Share this post


Link to post

"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;

 

  • Haha 1

Share this post


Link to post
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;

 

image.thumb.png.770f1371112c9c0595df2b58efba0c5d.png

Edited by Guest

Share this post


Link to post
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 :classic_biggrin: 

 

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!   

 

  • Haha 1

Share this post


Link to post
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 🙂

Share this post


Link to post
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 by David Schwartz

Share this post


Link to post
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.

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×