Berocoder 13 Posted May 25, 2023 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 Share this post Link to post
David Heffernan 2330 Posted May 25, 2023 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. Share this post Link to post
dummzeuch 1456 Posted May 25, 2023 (edited) 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 Share this post Link to post
Berocoder 13 Posted May 25, 2023 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 🙂 Share this post Link to post
Fr0sT.Brutal 898 Posted May 25, 2023 (edited) 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 1 Share this post Link to post
Fred Ahrens 58 Posted May 25, 2023 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. Share this post Link to post
dummzeuch 1456 Posted May 25, 2023 (edited) 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 Share this post Link to post
David Heffernan 2330 Posted May 25, 2023 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? 1 1 Share this post Link to post
David Heffernan 2330 Posted May 25, 2023 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. 5 Share this post Link to post
shineworld 73 Posted May 25, 2023 (edited) 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 2 Share this post Link to post
Tommi Prami 125 Posted May 25, 2023 Register methods to Dictionary, maybe? Tehn just bet the method to call from Dictionary by type? -Tee- Share this post Link to post
Clément 146 Posted May 25, 2023 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.. 1 Share this post Link to post
Stefan Glienke 1974 Posted May 25, 2023 Turn the strings into an enum or an index and put the functions into a const array over that enum/index. 2 Share this post Link to post
Brandon Staggs 231 Posted May 25, 2023 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. Share this post Link to post
David Heffernan 2330 Posted May 25, 2023 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. Share this post Link to post
Brandon Staggs 231 Posted May 25, 2023 (edited) 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 1 Share this post Link to post
programmerdelphi2k 237 Posted May 25, 2023 (edited) 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 Share this post Link to post
Fr0sT.Brutal 898 Posted May 26, 2023 (edited) 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 Share this post Link to post
Clément 146 Posted May 26, 2023 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! Share this post Link to post
programmerdelphi2k 237 Posted June 1, 2023 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; Share this post Link to post