Tuesday, May 20, 2014

I Spy Bad Code – Change Screen.Cursor

Recently, I have seen following code appeared again and again in one of application written in Delphi:

begin

      …
  Screen.Cursor := crHourGlass;
  try
   …

  except
    Screen.Cursor := crDefault;
    …
  end; {try}
  Screen.Cursor := crDefault;
end;

This code works in most of the time, but it does have one issue, it makes an assumption that the cursor was crDefault thus it change the cursor back to default before the end. What if the cursor is not default before entering this code block? We can improve the code as following thus it will restore the cursor to what it is before this code block:

begin

       …

      LSavedCursor  := Screen.Cursor;
  Screen.Cursor := crHourGlass;
  try
    …

  except   
    Screen.Cursor := LSavedCursor;
    …
  end; {try}
  Screen.Cursor := LSavedCursor;
end;

Or more proper way (in my opinion) is to use a try … finally block to protect the code block and it will guarantee the cursor will be restored:

begin

      …

      LSavedCursor := Screen.Cursor;
  Screen.Cursor := crHourGlass;
  try
    try
      …  //doing busy business code insert here

    except
      …
    end; {try}

  finally
    Screen.Cursor := LSavedCursor;

  end;
end;

Can it be improved further? The answer is YES and NO. If you are still using Delphi earlier than Delphi 2009, then the answer is NO, because we are going to use Anonymous Method which was introduced by Delphi 2009 and later version of Delphi.

With above code, you actually need to define the local variable LSaveCursor in each method in which it has above code, so it is a bit trivial, and you cannot refactoring above code to a method because the actually doing busy business code is vary from method to method, refactoring each doing busy business code to a method and then pass the method’s point will be a over kill approach.  But with anonymous method, we don’t need to refactoring doing busy business code to a method, instead, we can pass the code as an anonymous method.

First we need to create a procedure to take care the logic for changing the cursor to hour glass and restore it when it completes. Below is the procedure:

procedure BusyDoing (AProc:TProc);
var
  LSavedCursor : TCursor;
begin
  LSavedCursor := Screen.Cursor;
  Screen.Cursor := crHourGlass;
  try
    AProc;
  finally
    Screen.Cursor := LSavedCursor;
  end;
end;

This procedure has one parameter AProc which is a pre-defined anonymous method which is defined in unit System.SysUtils as

TProc = reference to procedure;

To use this procedure, all we need to do is enclose the doing busy business code as an anonymous method like this:

BusyDoing(
    procedure
      begin
           … //doing busy business code insert here

      end
);

That is it, very neat, isn’t it?