[Delphi] How would you refactor this code?

Question

This hypothetical example illustrates several problems I can't seem to get past, even though I keep trying!! ... Suppose the original code is a long event handler, coded in the UI, triggered when a user clicks a cell in a grid. Expressed as pseudocode it's:

if Condition1=true then
begin
  //loop through every cell in row, 
  //if aCell/headerCellValue>1 then
  //color aCell red
end
else if Condition2=true then
begin
  //do some other calculation adding cell and headerCell values, and
  //if some other product>2 then
  //color the whole row green
end
else show an error message

I look at this and say "Ah, refactor to the strategy pattern! The code will be easier to understand, easier to debug, and easier to later extend!" I get that.

And I can easily break the code into multiple procedures.

The problem is ultimately scope related. Assume the pseudocode makes extensive use of grid properties, values displayed in cells, maybe even built-in grid methods. How do you move all that to another unit, without referencing the grid component in the UI--which would break all the "rules" about loose coupling that make OOP valuable? ...

I'm really looking forward to responses. Thanks, as always -- Al C.

Answer 1

First of all, NEVER EVER compare a value to true. It's

if Condition1 then
begin
end else if Condition2 then
begin
end;

Comparing to true can, in the worst case, fail even if the value is true. There is a good (but german) article 'Über den Umgang mit Boolean' in the Delphi-PRAXiS.net community forums wich shows an example to reproduce this strange-seeming behaviour.

Regarding your question directly:

This code would be better in a custom paint event. This will get called for every cell and directly paint it with the correct colors. And it well draw in the correct colors even when repainted. In your event you would only draw the cells once, and if - for any reason - the grid repaints your color would be lost.

Then, after all, this code is stringly UI and component related. If you hadn't this grid on your form, you wouldn't need this code. What you could do to decouple things a bit is to pass the values retrieved from the grid row to an external unit that will do only the calculation and returns a logical result. Your UI code on the form then would take this result and has to decide how it has to be displayed (ie. what color etc.) and put the information on the grid.

Answer 2

Refactoring to put code into a separate routine doesn't necessarily mean decoupling everything. You could just as well refactor each of those cases into a new method belonging to the same class as the event handler you're refactoring. Those methods would have all the same access to the grid component as your current code already has.

You're writing code for that event to do things to that grid on that form. Do you really foresee needing to do those operations in response to some other event? Or perform them on some other grid on some other form? If not, then decoupling everything is just an academic exercise and serves no purpose to your product. It's OK to write application-specific code.

If you want to decouple, then the way to do it is to add parameters to your factored-out routines. If the routines need to work with the grid without knowing exactly which grid it is, then pass the grid in as a parameter:

if Condition1 then
  ColorCellsRedAboveRatio(Grid, 1.0)
else if Condition2 then
  ColorRowsGreenAboveProduct(Grid, 2)
else
  Error;

Answer 3

You could consider to extract an UI dependent unit. If the method is long and you want to extract some stuff from the class, it is reasonable to extract a strategy.

As Rob suggested you could just pass the needed context to the strategy procedures. You could introduce a SheetRenderingStrategy with an approriate abstract method and approriate SubClasses eg. HighlightTheseSpecialCellsStrategy. These classes are still part of the UI, but probably clarifiy the intent and improve modularisation.

Answer 4

Answering a question about refactoring, without context, is like answering a question about design, without context. Because you are changing a design. I usually start out with "reasons to refactor". Maybe I have metrics that I'm exceeding. (Look, a class with 10,000 lines, surely it can be partitioned into several more-coherent, more-cohesive classes, with less coupling, and tighter cohesion).

So, if you find yourself with a lot of code that has several hundred if...else conditions in event handlers, as I often do, I would forget all about that event handler for a minute, and reduce it as the person said above, to a minimal object oriented pattern:

if a then doA(fewer,parameters) else if b then doB(is,generally,better) else if c then doC; ...

Now, if doA,doB,and doC belong together in another object (They share state, and modify/control some particular set of fields), then I might move doA,doB,doC methods into another object.

In general, however, rather than drilling down to an individual case-by-case where event handlers do everything, I also find the following delphi-pattern handy:

procedure TForm1.BigGuiControlRightButtonClick(Sender;...);
begin
     BigThingController.RightClickMenuHandler(Sender, ....)
end;

procedure TForm1.BigGuiControlDoSomeThing(Sender:TObject);
begin
    BigThingController.DoSomeThing;
end;

procedure TForm1.Print(Sender);
begin
     DocumentManager.Print(Document);
end;

I like it when my TForm methods are clear and readable. I don't like to see a lot of noise, and a lot of error-checking code. I find that applications that have been carefully maintained and debugged over years tend to grow iteratively towards a complete mess of unreadable spaghetti. If the goal of refactoring is more than just to make the code look pretty, then the refactoring should also have some measurable quality goal too. Reduce defects, crashes, etc. Sometimes, I use refactoring as a time to remove features that are no longer useful, or implemented in a faulty way. So my code is more correct when I'm finished, and not just refactored to fit some ideal of how code should be written, that doesn't change the quality the user experiences. I'm a delphi developer, and I'm goal oriented, and quality oriented, and pragmatic rather than a stylizer. Other people may differ here.