Which implementation of this is easier to understand?

[dummzeuch 1505]

The following method is supposed to find the first entry that has a FReader.Data.Time <> 0 and return this Time and the associated MeasurementTime.

If it doesn't find an entry, it raises an EMyException.

FReader is an object to access records like in an array or a file of record, Seek(Idx) selects the record at the Idx'th position.

 

Which implementation do you think is easier to understand?

procedure TBla.SearchValidEntry(out _UtcValid, _TimeByUtc: TMeasurementTime);
var
  i: Integer;
  Utc: TMeasurementTime;
  IsValid: Boolean;
begin
  IsValid := False;
  for i := 0 to FReader.Count - 1 do begin
    FReader.Seek(i);
    Utc := FReader.Data.Time;
    if Utc.IsZero then begin
      Continue;
    end else begin
      _UtcValid := Utc;
      _TimeByUtc := FReader.MeasurementTime(i);
      IsValid := True;
      Break;
    end;
  end;
  if not IsValid then begin
    raise EMyException.CreateFmt(_('%s has no valid times'), [FReader.DataFilename]);
  end;
end;

vs.

procedure TBla.SearchValidEntry(out _UtcValid, _TimeByUtc: TMeasurementTime);
var
  i: Integer;
  Utc: TMeasurementTime;
begin
  for i := 0 to FReader.Count - 1 do begin
    FReader.Seek(i);
    Utc := FReader.Data.Time;
    if not Utc.IsZero then begin
      _UtcValid := Utc;
      _TimeByUtc := FReader.MeasurementTime(i);
      Exit;
    end;
  end;
  raise EMyException.CreateFmt(_('%s has no valid entry'), [FReader.DataFilename]);
end;

I realize that both use "Anty-Patterns", the first Break and Continue in the for loop, the second an Exit in the for loop.

Could the implementation be improved?

[Lars Fosdal 1792]

I'd go for the second alternative, but I'd do it as a function returning a boolean for found/not found and an optional boolean flag on whether to raise an exception or not.

[Der schöne Günther 316]

I'd have expected your method to return a Boolean for indication whether something has been found, and the out parameter can be used (your "IsValid").

An unsuccessful search isn't that out of the ordinary, so an exception is not appropriate.

 

I also don't see that the first is an anti-pattern. Loop control with break and continue is perfectly fine. Did I miss something?

 

 

 

[Uwe Raabe 2057]

While reading the first one I thought that I would implement that with an Exit instead of using the IsValid variable. So I vote for the second. Less code, less problems.

Just now, Der schöne Günther said:

An unsuccessful search isn't that out of the ordinary

Well, only Thomas can know that.

[Alexander Elagin 143]

2 minutes ago, Der schöne Günther said:

I also don't see that the first is an anti-pattern. Loop control with break and continue is perfectly fine. Did I miss something?

A redundant begin/end block around a single Continue, maybe? It just increases the number of lines and clutters the view, IMHO.

    if Utc.IsZero then Continue
    else begin
      _UtcValid := Utc;
      ...

[Kryvich 165]

7 minutes ago, Alexander Elagin said:

A redundant begin/end block around a single Continue

The code can be further simplified:

    if Utc.IsZero then 
      Continue;
    _UtcValid := Utc;
    ...

 

[dummzeuch 1505]

1 hour ago, Lars Fosdal said:

I'd go for the second alternative, but I'd do it as a function returning a boolean for found/not found and an optional boolean flag on whether to raise an exception or not.

Good Point. In that case I'd would have two methods:

1. a function TryXxxx which returns a boolean
2. a proceuder Xxxx which calls TryXxxx and raises an exception if that returns false

[dummzeuch 1505]

2 hours ago, Der schöne Günther said:

I'd have expected your method to return a Boolean for indication whether something has been found, and the out parameter can be used (your "IsValid").

An unsuccessful search isn't that out of the ordinary, so an exception is not appropriate.

In general, you are right. In that case I would have called the function TryGetFirstValidEntry or something similar.

In this particular case it is an error if there is no valid entry. This method was only added because the first entry did not have a valid time which caused an error elsewhere. We added a search for the first valid entry because of that. If there is none, some manual action is required.

Thinking about this: The method should probably be called GetFirstValidEntry rather than SearchValidEntry.

2 hours ago, Der schöne Günther said:

I also don't see that the first is an anti-pattern. Loop control with break and continue is perfectly fine. Did I miss something?

Break and Continue are considered special cases of Goto by some (and I tend to agree because they usually decrease readability).

Edited October 20, 2021 by dummzeuch

[Uwe Raabe 2057]

17 minutes ago, dummzeuch said:

because they usually decrease readability

I don't share this opinion, but hey, readability is highly subjective, isn't it?

[Fr0sT.Brutal 900]

Choosing between several Break/Continue/Exit's and numerous nested begin-end's, I prefer the former.

procedure TBla.SearchValidEntry(out _UtcValid, _TimeByUtc: TMeasurementTime);
var
  i: Integer;
  Utc: TMeasurementTime;
begin
  for i := 0 to FReader.Count - 1 do begin
    FReader.Seek(i);
    Utc := FReader.Data.Time;
    if Utc.IsZero then 
      Continue;
    _UtcValid := Utc;
    _TimeByUtc := FReader.MeasurementTime(i);
    Exit;
  end;
  raise EMyException.CreateFmt(_('%s has no valid entry'), [FReader.DataFilename]);
end;

I'd do it as above

Edited October 20, 2021 by Fr0sT.Brutal

[Attila Kovacs 629]

The 2nd implementation is better for my eyes.

 

 

Edited October 20, 2021 by Attila Kovacs

[David Heffernan 2345]

3 hours ago, dummzeuch said:

the second an Exit in the for loop.

Not an anti pattern

[Stefan Glienke 2002]

Second but without Egyptian begin/end.

[Fr0sT.Brutal 900]

40 minutes ago, Stefan Glienke said:

Egyptian begin/end

Awesome! 😄

[0x8000FFFF 22]

43 minutes ago, Stefan Glienke said:

Egyptian begin/end.

First time I see this phrase. Makes sense.

Edited October 20, 2021 by 0x8000FFFF

[dummzeuch 1505]

I for one prefer "Egyptian begin/end". And I like the name. 😉

[Rollo62 536]

9 hours ago, dummzeuch said:

procedure TBla.SearchValidEntry(out _UtcValid, _TimeByUtc: TMeasurementTime);
var
  i: Integer;
  Utc: TMeasurementTime;
begin
  for i := 0 to FReader.Count - 1 do begin
    FReader.Seek(i);
    Utc := FReader.Data.Time;
    if not Utc.IsZero then begin
      _UtcValid := Utc;
      _TimeByUtc := FReader.MeasurementTime(i);
      
             <####=======================================================================================##
      Exit;  <#### R E A D M E:      !! I EXIT HERE !!                                                 ==##
             <###========================================================================================##
             
    end;
  end;
  raise EMyException.CreateFmt(_('%s has no valid entry'), [FReader.DataFilename]);
end;

I realize that both use "Anty-Patterns", the second an Exit in the for loop.

I prefer 2 as well.

But in case of AntiPattern, I like to make "shouting" comments showing me where the problem sits.

Make it as as much as "shouting" as you like, its more a question of "taste" (  Ok, I'm overdoing a bit in the upper example ). 

 

Are these "shouting" comments another AntiPattern maybe ? 🤔

 

Edited October 20, 2021 by Rollo62

[dummzeuch 1505]

14 minutes ago, Rollo62 said:

"crying" comments

I think "shouting" would be a better word for what you mean. But I agree. My Comment usually looks like this:

if bla then
  Exit; //==>

Not quite as "loud" but at least a bit more visible than a simple "Exit";

Of course, given structural highlighting this is not quite as important nowadays as it was on the olden days.

[Rollo62 536]

@dummzeuch

Done. You're perfectly right.

At this time of the day my english translation brain sub-module doesn't work so well anymore.

I have to wait until next morning, if I'm lucky

[FredS 138]

1 hour ago, dummzeuch said:

a bit more visible than a simple "Exit"

Stopped that after the 'Flow control Highlights' became available, at least in IDEs that support it.
 

Edited October 20, 2021 by FredS

[Rollo62 536]

2 hours ago, FredS said:

Stopped that after the 'Flow control Highlights' became available, at least in IDEs that support it.

You mean these little red thingies ?

 

Not visible enough for me, I use standard lines like this, as my default

[FredS 138]

48 minutes ago, Rollo62 said:

Not visible enough for me

This setup works for me:

 

[David Schwartz 426]

I like the second simply because it's shorter. I also wish there was a better mechanism available but suspect it would end up looking rather like a Case statement, sort of like Awk scripts.

 

I don't know why any of these are anti-patterns because the alternative is ugly-patterns that are harder to read and add unnecessary logic.

 

You could change it to a while or repeat...until structure, but they both require an initial probe at the top; while requires some kind of 'get-next' at the bottom of the loop; and repeat...until always seems to need a check near the bottom for make-or-break unless you want to add another variable to the loop test, which always seems redundant to me.

 

This is the problem: you have two independent variables involved: one controls the loop, and the other interupts it if it finds a resolution before the whole sample set has been exhausted, which only improves performance.

 

To be more precise with your expressions, the for loop would contain two tests, one for each variable. This is easy in C/C++ for loops, but not in Delphi/Pascal.

 

Instead, you'd need a while or repeat...until. But as I said, you can only initilize and work with one variable in the for statement in Pascal; again, C/C++ makes it easy.

 

So Pascal leaves us with a half-assed solution, exemplified by this:
 

var sl := TStringlist.Create;

// add a bunch of things with objects to the list
...
   sl.AddObject( aString, someObject );
   // this deceptive call makes it look like there's a relationshiop between aString and someObject, 
   // but there actually isn't because they have separate indices!
   // They DO happen to coincide, however. 
   // Sadly, neither sl[aString] nor sl[aString].Object leads to the corresonding entry in Objects that yields this pair.
...

// now find and return an object
function findSomething( const aStringArg : string ) : TObject;
begin
   Result := NIL;
   for var str in sl do
      if (str = aStringArg) then
         Exit( sl.Objects[ ??? ];
         // oops ... no way to get the corresponding item in Objects!
    
   // you are forced to go this way instead
   for var i := 0 to sl.Count-1 do
      if (aStringArg = sl[i]) then
   	     Exit( sl.Objects[i] );
end;

The for...in is a much cleaner way to go, but the fact that the embedded Objects array requires the item's index to access its contents mucks things up.

 

So you need to drag in a separate index just to satisfy that need.

 

You could say this is a fault in the design of TStringlist, but it's a widespread problem.

 

Again, C/C++ solves it nicely by allowing multiple independent variables to be used in a For loop.

 

Perhaps for TStringlists, it would be better if the iterator had a hidden index that could be used to probe into the Objects array:

 

for aStr in sl do
   if (aStr = sStringArg) then
      Exit( sl.Selected.$Object ); // Selected would be a new property introduced into the iterator allowing the 
                                   // Object property to find the index associated with this entry to extract a value
      

So maybe it's a limitation in how Deplhi implements iterators rather than TStringlists (and every other class that has a similar problem).

 

 

 

Edited October 20, 2021 by David Schwartz

[Stefan Glienke 2002]

Wrong, string and object are in the same entry - the internal storage of a TStringList has an array of TStringItem which is a record which contains a string and an object.

 

You simply find it like this:

 

function findSomething( const aStringArg : string ) : TObject;
var
  i: Integer;
begin
  i := sl.IndexOf(aStringArg);
  if i >= 0 then
    Result := sl.Objects[i]
  else
    Result := nil;
end;

Make this a method in your own TStringListHelper and there you go. If you want easy and fast lookup of objects by string key, then use a dictionary.

 

[Darian Miller 361]

Since the actions all involve FReader, I'd move the method into FReader's class instead of TBla.  I think it makes it much more readable (as the general rule of thumb is - the fewer dots the better)

Also changed the method name as you suggested in the thread.

 

Bonus issues:

Changed Utc.IsZero to a new Utc.IsValidTimeEntry as that logic belongs to TMeasurementTime, not this method.

Depending on the nature of the Reader, you might need to save the current position and restore it afterwards with a try/finally block to squash any changed-state issues.

Also changed to use a Exception class scoped to the Reader.

'Seek' seems a little oddly named method here... more like a 'Goto' or RecordNumber assignment.  (Maybe add a custom enumerator and use FOR/IN and get rid of the method call)

Might also make a resource string for the exception text.

procedure TReaderClass.GetFirstValidTimeEntry(out _UtcValid, _TimeByUtc:TMeasurementTime);
var
  i:Integer;
  Utc:TMeasurementTime;
begin
  for i := 0 to Count - 1 do
  begin
    Seek(i);
    Utc := Data.Time;
    if not Utc.IsValidTimeEntry then
    begin
      _UtcValid := Utc;
      _TimeByUtc := MeasurementTime(i);
      Exit;
    end;
  end;
  raise EMyReaderException.CreateFmt('%s has no valid entry', [DataFileName]);
end;

 

Edited October 21, 2021 by Darian Miller