Legacy code snippet of the day:

-
Legacy code snippet of the day:
(Yes.  This is found in real production code.)

   // Create form
   case Nr of
     1: OpneForm(Fil, False);
     2: OpneForm(Fil, False);
     3: OpneForm(Fil, False);
     4: OpneForm(Fil, False);
     5: OpneForm(Fil, False);
     6: OpneForm(Fil, False);
     7: OpneForm(Fil, False);
     8: OpneForm(Fil, False);
     9: OpneForm(Fil, False);
   end;

Comments

  1. Nándor KissFebruary 19, 2015 at 10:56 AM

    :D

    ReplyDelete
  2. Primož GabrijelčičFebruary 19, 2015 at 11:00 AM

    What about 0? He forgot to handle the Nr = 0 case!

    ReplyDelete
  3. Stefan GlienkeFebruary 19, 2015 at 11:11 AM

    Sourcecode in RAID 5? ^^

    ReplyDelete
  4. Heinz ToskanoFebruary 19, 2015 at 11:18 AM

    Primož Gabrijelčič what  about any other case? not just Nr = 0!

    ReplyDelete
  5. Thomas Mueller (dummzeuch)February 19, 2015 at 11:32 AM

    Maybe there used to be different forms at some time and one after another became obsolete?

    ReplyDelete
  6. Stefan GlienkeFebruary 19, 2015 at 11:34 AM

    Removing redundant or obsolete code - my favorite refactoring

    ReplyDelete
  7. Uwe RaabeFebruary 19, 2015 at 11:36 AM

    Perhaps it is meant for Multithreading?

    ReplyDelete
  8. Daniela OsterhagenFebruary 19, 2015 at 11:45 AM

    Uwe Raabe how would multithreading make this any more reasonable?

    ReplyDelete
  9. Uwe RaabeFebruary 19, 2015 at 12:02 PM

    Daniela Osterhagen How would anything make this anymore reasonable?

    ReplyDelete
  10. Thomas Mueller (dummzeuch)February 19, 2015 at 12:58 PM

    Uwe Raabe Isn't that what Daniela was asking?

    ReplyDelete
  11. Stefan GlienkeFebruary 19, 2015 at 1:00 PM

    I guess Uwe is playing "stupid programmers advocate" here trying to find reasons why this code exists ;)

    ReplyDelete
  12. Lars FosdalFebruary 19, 2015 at 3:14 PM

    It could be a breakpoint construct. Break on fifth file opened.  yes, you can do that with conditions - but who knows...

    ReplyDelete
  13. Lars FosdalFebruary 19, 2015 at 3:16 PM

    Jørn Einar Angeltveit​ where does nr come from?

    ReplyDelete
  14. Steve RileyFebruary 19, 2015 at 5:46 PM

    Best part is the "//Create form" comment

    ReplyDelete
  15. Heinz ToskanoFebruary 19, 2015 at 5:49 PM

    no one notice the "typo" in "OpneForm"?

    ReplyDelete
  16. Thomas Mueller (dummzeuch)February 19, 2015 at 9:41 PM

    Heinz Toskano I was assuming that it is some foreign language.

    ReplyDelete
  17. Lars FosdalFebruary 19, 2015 at 11:47 PM

    Opne is a valid dialect form of Open in Norwegian.

    ReplyDelete
  18. mezenFebruary 20, 2015 at 1:35 AM

    Steve Riley yap, the comment is the best part xD

    ReplyDelete
  19. Lars FosdalFebruary 20, 2015 at 1:43 AM

    I can "improve" on that ...

       // Create form
       case Nr of
         0: ; // Ignore this
         else OpneForm(Fil, False);
       end;

    ReplyDelete
  20. Nándor KissFebruary 20, 2015 at 1:46 AM

    "Should" be this, no? :)

     // Create form
       if (Nr in [1..9]) then
         OpneForm(Fil, False);

    ReplyDelete
  21. Sébastien ParadisFebruary 20, 2015 at 2:45 AM

    This is already refactored version; 1st it was like :
    if Nr = 1 then
      OpneForm(Fil, False)
    else if Nr = 2 then
      OpneForm(Fil, False)
    else if Nr =3 then
      OpneForm(Fil, False)
    else if Nr = 4 then
      OpneForm(Fil, False)
    etc. :)

    ReplyDelete
  22. Thomas Mueller (dummzeuch)February 20, 2015 at 4:04 AM

    Simon Stuart if you raise an exception in the if branch, why have an else branch at all?

    ReplyDelete
  23. Thomas Mueller (dummzeuch)February 20, 2015 at 4:15 AM

    So why raise an exception. You could just show a messagebox and terminate the program with Halt(4711). I have seen quite a lot programs doing that, so it must be good practice.

    ReplyDelete
  24. Lars FosdalFebruary 20, 2015 at 4:58 AM

    We could modernize it...

    type
      TOpneAction = reference to procedure;

      TOpneFormFactory = class(TDictionary    procedure Populate(const aIndexes: array of Integer; const aAction:TOpneAction);
        procedure Opne(const aIndex: Integer);
      end;

    procedure TOpneFormFactory.Populate(const aIndexes: array of Integer; const aAction:TOpneAction);
    var
      ix: Integer;
    begin
      for ix in aIndexes
       do Add(ix, aAction)
    end;

    procedure TOpneFormFactory.Opne(const aIndex: Integer);
    var
      Action: TOpneAction;
    begin
      if TryGetValue(aIndex, Action)
       then Action
        else raise Exception.Create('Call 911'); 
    end;

    ...

    var
      OpneFactory: TOpneFormFactory;

    begin
      OpneFactory := TOpneFormFactory.Create;
      OpneFactory.Populate([1,2,3,4,5,6,7,8,9],
        procedure
        begin
           OpneForm(Fil, False);
        end);

    ...

      // Create form
      OpneFactory.Opne(Nr);

    ReplyDelete
  25. Bill MeyerFebruary 20, 2015 at 5:01 AM

    Lars Fosdal Oh, well done.

    ReplyDelete
  26. AvatarxFebruary 20, 2015 at 6:42 AM

    No exception catch from all 1..9 Forms :)

    ReplyDelete
  27. Eric GrangeFebruary 20, 2015 at 6:44 AM

    Lars Fosdal The scary bit is that modernized code looks from afar like any other snippet of modernized code, and not like a WTF... even though it still is the same WTF.

    ReplyDelete
  28. Stefan GlienkeFebruary 20, 2015 at 6:48 AM

    Lars Fosdal Well done! Now that's real enterprise code! ;p

    ReplyDelete
  29. Bill MeyerFebruary 20, 2015 at 6:58 AM

    Stefan Glienke I won't say what sort of enterprise....

    ReplyDelete
  30. Lars FosdalFebruary 20, 2015 at 7:20 AM

    Bill Meyer - Clearly, the starship variant ... i.e. far out...

    ReplyDelete
  31. Kerry SandersFebruary 21, 2015 at 3:12 PM

    Simon Stuart Your response is just about like what I was going to post and one that Bill Meyer can really appreciate. I really cannot improve on it, but then again, what I am referring to, there would not have been the raise exception. :-)

    ReplyDelete

Post a Comment