Use case or if else ?

[Berocoder 14]

What code to you prefer and why?

Alternative 1

  if aMessageType = 'EXPREG' then
    DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPACC' then
    DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPERR' then
    DecodeExportErr(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPREL' then
    DecodeExportRel(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPREF' then
    DecodeExportRef(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPREQ' then
    DecodeExportReq(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPINF' then
    DecodeExportInf(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPREJ' then
    DecodeExportRej(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPCLA' then
    DecodeExportCla(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPCON' then
    DecodeExportCon(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPAME' then
    DecodeExportAme(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPIRJ' then
    DecodeExportIrj(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPARJ' then
    DecodeExportArj(vDoc, aCdn, aRequestCorrelation, aMessageText)
  else if aMessageType = 'EXPHRT' then
    DecodeExportHrt(vDoc, aCdn, aRequestCorrelation, aMessageText)

 

Alternative 2

  var Index := IndexStr(aMessageType, ['EXPREG', 'EXPACC', 'EXPERR', 'EXPREL', 'EXPREF', 'EXPREQ', 'EXPINF', 'EXPCLA', 'EXPCON', 'EXPAME', 'EXPIRJ', 'EXPARJ', 'EXPHRT']);
  case Index of
    0: DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText);
    1: DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText);
    2: DecodeExportErr(vDoc, aCdn, aRequestCorrelation, aMessageText);
    3: DecodeExportRel(vDoc, aCdn, aRequestCorrelation, aMessageText);
    4: DecodeExportRef(vDoc, aCdn, aRequestCorrelation, aMessageText);
    5: DecodeExportReq(vDoc, aCdn, aRequestCorrelation, aMessageText);
    6: DecodeExportInf(vDoc, aCdn, aRequestCorrelation, aMessageText);
    7: DecodeExportRej(vDoc, aCdn, aRequestCorrelation, aMessageText);
    8: DecodeExportCla(vDoc, aCdn, aRequestCorrelation, aMessageText);
    9: DecodeExportCon(vDoc, aCdn, aRequestCorrelation, aMessageText);
    10: DecodeExportAme(vDoc, aCdn, aRequestCorrelation, aMessageText);
    11: DecodeExportIrj(vDoc, aCdn, aRequestCorrelation, aMessageText);
    12: DecodeExportArj(vDoc, aCdn, aRequestCorrelation, aMessageText);
    13: DecodeExportHrt(vDoc, aCdn, aRequestCorrelation, aMessageText);
  end;

 

Regards

Roland Bengtsson

[David Heffernan 2345]

The if is better. But a better comparison would be a version that converted the string to an enum. That would be more readable in the long case statement. 

[dummzeuch 1505]

I'm not sure how efficient the implementation of IndexStr is. Assuming it's a linear search, the case statement is worse on two accounts:

1. It more difficult to read and understand.

2. It's a bit less efficient.

 

On top of that this implementation is error prone. If somebody inserts a new string rather than appending it to the array the indices in the case statement will all have to be corrected.

 

It has only one advantage: It's less to type. And of course, it looks cleaner.

 

Some kind of sorted list or hash table storing the strings and an enum value would be more efficient and less error prone (edit: in short: a dictionary). Of course that list must be initialized before it can be used, preferably only once.

 

People who are used to languages that support case statements with strings will of course laugh at that code anyway.

Edited May 25, 2023 by dummzeuch

[Berocoder 14]

39 minutes ago, dummzeuch said:

I'm not sure how efficient the implementation of IndexStr is. Assuming it's a linear search, the case statement is worse on two accounts:

1. It more difficult to read and understand.

2. It's a bit less efficient.

 

On top of that this implementation is error prone. If somebody inserts a new string rather than appending it to the array the indices in the case statement will all have to be corrected.

 

It has only one advantage: It's less to type. And of course, it looks cleaner.

 

Some kind of sorted list or hash table storing the strings and an enum value would be more efficient and less error prone (edit: in short: a dictionary). Of course that list must be initialized before or can be used, preferably only once.

 

People who are used to languages that support case statements with strings will of course laugh at that code anyway.

I would prefer alternative 2. But you are right that it is error prone on change as it is harder to read.

And as David said using enum would be best choice.

Currently alternative 1 is used so I think I leave it as is for now 🙂

 

[Fr0sT.Brutal 900]

TEnum + array[TEnum] of strings + case TEnum(IndexStr) =>

1. Constants are extracted from code

2. array[TEnum] ensure constants are in sync with enum

3. named enum members are more readable than numbers

4. static analyzer (alas not the Delphi's) will how warning if case won't cover all possible enum values

 

I used IndexStr approach in some of inline switches but it really doesn't look obvious.

If this case is speed-critical, you probably have to use sorted array and binary search. However for low number of items it gains almost nothing. Dictionary also has its penalty of hashing the value to be searched for. In fact seems for short sets of short strings nothing beats dumb linear search in an array

Edited May 25, 2023 by Fr0sT.Brutal

[Fred Ahrens 59]

I prefer Alternative 1 but I wouldn't use "else". Using "else" might slightly improve performance in some cases, but without the "else" I think the code can be parsed better during code reviews (gets closer to the readability of the version using the case statement).

if aMessageType = 'EXPREG' then
  DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPACC' then
  DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPERR' then
  DecodeExportErr(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREL' then
  DecodeExportRel(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREF' then
  DecodeExportRef(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREQ' then
  DecodeExportReq(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPINF' then
  DecodeExportInf(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREJ' then
  DecodeExportRej(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPCLA' then
  DecodeExportCla(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPCON' then
  DecodeExportCon(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPAME' then
  DecodeExportAme(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPIRJ' then
  DecodeExportIrj(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPARJ' then
  DecodeExportArj(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPHRT' then
  DecodeExportHrt(vDoc, aCdn, aRequestCorrelation, aMessageText);

OT: just learned about the existence of IndexStr. 

[dummzeuch 1505]

9 minutes ago, Fred Ahrens said:

I prefer Alternative 1 but I wouldn't use "else". Using "else" might slightly improve performance in some cases, but without the "else" I think the code can be parsed better during code reviews (gets closer to the readability of the version using the case statement).

Unfortunately that would allow for yet another possible problem: What if two or more of these strings were identical? In that case multiple different DecodeExportXxx calls would be made:

if aMessageType = 'EXPREG' then
  DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREG' then // <== typo here
  DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText);

When using "else if", in theory the compiler could create a warning in that case because the second if for that string could never be executed. But I doubt that the Delphi compiler actually is that good at analysing the code.

if aMessageType = 'EXPREG' then begin
  DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText);
end else if aMessageType = 'EXPREG' then begin
  DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText); // <== possible compiler warning here
end;

 

Edited May 25, 2023 by dummzeuch

[David Heffernan 2345]

19 minutes ago, Fred Ahrens said:

I prefer Alternative 1 but I wouldn't use "else". Using "else" might slightly improve performance in some cases, but without the "else" I think the code can be parsed better during code reviews (gets closer to the readability of the version using the case statement).

if aMessageType = 'EXPREG' then
  DecodeExportReg(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPACC' then
  DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPERR' then
  DecodeExportErr(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREL' then
  DecodeExportRel(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREF' then
  DecodeExportRef(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREQ' then
  DecodeExportReq(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPINF' then
  DecodeExportInf(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPREJ' then
  DecodeExportRej(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPCLA' then
  DecodeExportCla(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPCON' then
  DecodeExportCon(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPAME' then
  DecodeExportAme(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPIRJ' then
  DecodeExportIrj(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPARJ' then
  DecodeExportArj(vDoc, aCdn, aRequestCorrelation, aMessageText);
if aMessageType = 'EXPHRT' then
  DecodeExportHrt(vDoc, aCdn, aRequestCorrelation, aMessageText);

OT: just learned about the existence of IndexStr. 

Why perform 7 comparisons, on average, when you can perform 14 every time?

[David Heffernan 2345]

It would also be better to write a function that took the string, and returned the method to be called, to be held in a variable of procedural type. You could then have a single call to the procedure where you passed the arguments.

 

It's very repetitious to pass the same 4 arguments to 14 separate methods.

[shineworld 73]

In this case, a "key":@method hash list (GpStringHash from Primoz Gabrijelcic) could be a practicable solution ?
I've used often...

Edited May 25, 2023 by shineworld

[Tommi Prami 130]

Register methods to Dictionary, maybe? Tehn  just bet the method to call from Dictionary by type?

 

-Tee-

[Clément 148]

I would also register my methods. They all seems to have the same signature.

 

unit Unit121;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs,
  Generics.Collections, Vcl.StdCtrls;

type
  TMyDecodeProcedure = procedure ( aDoc, aCdn, aRequestCorrelation, aMessageText : string ) of object;

  TForm121 = class(TForm)
    Button1: TButton;
    procedure FormCreate(Sender: TObject);
    procedure FormDestroy(Sender: TObject);
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
    fDecoders : TDictionary<String, TMyDecodeProcedure >;
    procedure RegisterDecodeFunc;

    procedure DecodeExportAcc( aDoc, aCdn, aRequestCorrelation, aMessageText : string);
    procedure DecodeExportErr( aDoc, aCdn, aRequestCorrelation, aMessageText : string );
    procedure DecodeExportReg( aDoc, aCdn, aRequestCorrelation, aMessageText : string );
  public
    { Public declarations }
    function Execute( aDecoder, aDoc, aCdn, aRequestCorrelation, aMessageText : string; var aErrMsg : String ) : Boolean;
  end;

var
  Form121: TForm121;

implementation

{$R *.dfm}

procedure TForm121.FormCreate(Sender: TObject);
begin
  fDecoders := TDictionary<String, TMyDecodeProcedure >.Create;
  RegisterDecodeFunc;
end;

procedure TForm121.FormDestroy(Sender: TObject);
begin
  fDecoders.Free;
end;

procedure TForm121.DecodeExportAcc(aDoc, aCdn, aRequestCorrelation,
  aMessageText: string);
begin
  //
end;

procedure TForm121.DecodeExportErr(aDoc, aCdn, aRequestCorrelation,
  aMessageText: string);
begin
  //
end;

procedure TForm121.DecodeExportReg(aDoc, aCdn, aRequestCorrelation,
  aMessageText: string);
begin
  //
end;

function TForm121.Execute(aDecoder, aDoc, aCdn, aRequestCorrelation,
  aMessageText: string; var aErrMsg : String): Boolean;
var
  lDecoder : TMyDecodeProcedure;
begin
  Result := fDecoders.TryGetValue(aDecoder, lDecoder);
  if Result then
     lDecoder( aDoc, aCdn, aRequestCorrelation, aMessageText )
  else
     aErrMsg := Format('Decoder not found: %s', [aDecoder] );
end;

procedure TForm121.RegisterDecodeFunc;
begin
   fDecoders.Add('EXPREG', DecodeExportReg );
   fDecoders.Add('EXPACC', DecodeExportAcc );
   fDecoders.Add('EXPERR', DecodeExportErr );
   { ... }
end;

procedure TForm121.Button1Click(Sender: TObject);
var
  lErrMsg : String;
begin
  lErrMsg := '';
  Execute('EXPREG','1','2','3','4', lErrMsg);
end;

end.

And you can add decoders more easily too..

[Stefan Glienke 2002]

Turn the strings into an enum or an index and put the functions into a const array over that enum/index.

[Brandon Staggs 278]

6 hours ago, Fr0sT.Brutal said:

In fact seems for short sets of short strings nothing beats dumb linear search in an array

Agreed. Just wondering, do you have a personal rule of thumb as to when you decide to switch to a dictionary or binary-searched array? 

 

I know, I know, test, don't guess. Just wondering how long you let your "if SameStr() else if SameStr()" blocks get before you decide to do something more performant.

[David Heffernan 2345]

11 minutes ago, Brandon Staggs said:

Agreed. Just wondering, do you have a personal rule of thumb as to when you decide to switch to a dictionary or binary-searched array? 

For me the choice is between linear search and dictionary. Unless there's a natural reason for the collection to be ordered.

[Brandon Staggs 278]

6 minutes ago, David Heffernan said:

For me the choice is between linear search and dictionary. Unless there's a natural reason for the collection to be ordered.

What I meant was, if I have this:

 

if SameStr(Val, 'kirk') then HandleKirk
else if SameStr(Val, 'picard') then HandlePicard
else if SameStr(Val, 'sisko') then HandleSisko;

If I am not likely to add any more captains, there is no reason to optimize this with procedure registration and any kind of array or dictionary. But if Paramount keeps churning out garbage with new captains, at some point this is going to get long enough that if it is called in a loop, I will want to make the procedure lookup more optimal than a string if if-then-elses.

 

For only three values, there is no advantage to registering these functions, because it is readable and there is no performance to be gained. But at some point performance will take a hit.

 

I was curious what a general rule of thumb is for that. I realize it is a broad, general question of "feeling" that would be downvoted on SO. :-)

Edited May 25, 2023 by Brandon Staggs

[programmerdelphi2k 237]

13 hours ago, Berocoder said:

What code to you prefer and why?

if the performance and reliability leaves you in doubt, or the code hurts your eyes, or , then redo it!

 

How about using "arrays" to store your procedures to invoke them later?  just 1 "IF"

• using "type" to better control!

implementation

{$R *.dfm}

type
  TProcHeader = procedure(aDoc, aCdn, aRequestCorrelation, aMessageText: string);
  TMyEnum     = (meEXPREG, meEXPACC, meEXPERR, meEXPREL, meEXPREF, meEXPREQ, meEXPINF, meEXPCLA, meEXPCON, meEXPAME, meEXPIRJ, meEXPARJ, meEXPHRT);
  TMyArrProcs = array [TMyEnum] of TProcHeader; // it will contains: how many elements was defined on TMyEnum !

procedure DecodeExportReg(aDoc, aCdn, aRequestCorrelation, aMessageText: string);
begin
  ShowMessage('DecodeExportReg'); // only if "meEXPREG" was used on array
end;

procedure DecodeExportAcc(vDoc, aCdn, aRequestCorrelation, aMessageText: string);
begin
  ShowMessage('DecodeExportAcc'); // only if "meEXPACC" was used on array
end;

procedure DecodeExportHRT(vDoc, aCdn, aRequestCorrelation, aMessageText: string);
begin
  ShowMessage('DecodeExportHrt'); // only if "meEXPHRT" was used on array
end;

// the "core"
procedure MyExecuteMyProcs(AArrProcs: TMyArrProcs; AEnum: TArray<TMyEnum>; aDoc, aCdn, ACorrelation, AMsg: string);
begin
  for var E in AEnum do
    if (@AArrProcs[E] <> nil) then    {just 1 "IF"}
      AArrProcs[E](aDoc, aCdn, ACorrelation, AMsg);
end;

procedure TForm1.Button1Click(Sender: TObject);
var
  LArrProcs: TMyArrProcs;
begin
  LArrProcs := Default (TMyArrProcs); // Important: force "nil" for all, to avoid random "value"!!!
  //
  // ----------------------------------------
  // try uncomment each one for test it!
  // ----------------------------------------
  // LArrProcs[meEXPREG] := DecodeExportReg;
  // LArrProcs[meEXPACC] := DecodeExportAcc;
  // ...
  // LArrProcs[meEXPHRT] := DecodeExportHRT;
  //
  // LArrProcs[meEXPREQ] := nil; 
  //
  // LArrProcs[meEXPREG]('doc', 'cdn', 'correlation', 'msgtext');  // executing directly the proc!
  //
  // how test it:
  //
  // MyExecuteMyProcs(LArrProcs, [meEXPREG, meEXPACC], 'doc', 'cdn', 'correlation', 'msgtext'); // 2 uncommented
  // MyExecuteMyProcs(LArrProcs, [meEXPACC], 'doc', 'cdn', 'correlation', 'msgtext');           // uncomment "meEXPREG", later "meEXPACC"
  // MyExecuteMyProcs(LArrProcs, [meEXPHRT], 'doc', 'cdn', 'correlation', 'msgtext');           // ...
  // MyExecuteMyProcs(LArrProcs, [], 'doc', 'cdn', 'correlation', 'msgtext'); // uncomment all or not!
  //
  // all uncommented, you define "enum" order to execute, dont worry same that no defined on LArrProcs!
  MyExecuteMyProcs(LArrProcs, [meEXPREG, meEXPINF, meEXPCLA, meEXPCON, meEXPHRT, meEXPACC], 'doc', 'cdn', 'correlation', 'msgtext');
end;

 

Edited May 25, 2023 by programmerdelphi2k

[Fr0sT.Brutal 900]

17 hours ago, Brandon Staggs said:

Just wondering, do you have a personal rule of thumb as to when you decide to switch to a dictionary or binary-searched array?

No strict rule, just the personal feeling. I also prefer as short as possible solutions for non-critical fragments, i.e. I won't create and maintain dictionary for a proc that is called rarely, just use array of const.

Where the speed REALLY matters (parsers), all beauty of code and extendability could be sacrificed so there could appear constructions like

case inp[4] of
  'A': // switch STRA1, STRABC, STRADEF
  'B': // switch STRB2, STRBCD, STRDXYZ

 

Edited May 26, 2023 by Fr0sT.Brutal

[Clément 148]

23 hours ago, Brandon Staggs said:

Agreed. Just wondering, do you have a personal rule of thumb as to when you decide to switch to a dictionary or binary-searched array? 

 

In order to decide between them I usually check a few things:
  1. "Staticness" : How often will upper bound change. Is the Index "Static".
  2. "Filledness" : How well will the structure be filled.
  3. Missed Index : How often will I hit one of those empty index, and if I hit what happens ( How easily can I deal with it )
  4. Element Owner : If the elements are instances "Who" will created them. "Who" will free them. Will "Who" be the same? 🙂


Indexed array is a great structure. Can be both performant and low size.

"Dictionary" has it's weight. I'll use them whenever it's benefit outweigh this cost.
Anyway.. I often use TDictionary or (TObjectDictionary)   because I rarely have static index. ex: GUID is the key to another structure.
In the example I gave, it is possible to add decoders at runtime ( from a DLL, or a database). And it wouldn't required a recompilation. Which very cool!
 

[programmerdelphi2k 237]

On 5/25/2023 at 3:12 PM, programmerdelphi2k said:

MyExecuteMyProcs(LArrProcs, [meEXPREG, meEXPINF, meEXPCLA, meEXPCON, meEXPHRT, meEXPACC], 'doc', 'cdn', 'correlation', 'msgtext');

here you can use a "Set" like:

 

type
	TMyEnum  = (meEXPREG, meEXPACC, meEXPERR, meEXPREL, meEXPREF, meEXPREQ, meEXPINF, meEXPCLA, meEXPCON, meEXPAME, meEXPIRJ, meEXPARJ, meEXPHRT);
	TMyEnums = set of TMyEnum;
...
var
	MyEnums:TMyEnums;
begin
	MyEnums :=[meEXPACC,meEXPREG];
	MyExecuteMyProcs(LArrProcs, MyEnums, 'doc', 'cdn', 'correlation', 'msgtext');
end;