7
\$\begingroup\$

I have produced a working menu component for a project I'm working on, but would like to reduce the amount of code used and improve the methods - especially in the Paint implementation - still further, I feel like the function I've used and the way I've passed the variables to it is a bit 'hacky' - IOW, I bodged it.

What methods could I use to increase the amount of encapsulation used in the drawing routine so I'm not defining variables in the Paint routine? The y2 variable in particular is irritating.

These are my drawing function and paint routine:

// Rectangle drawing function
Function DrawRect(cv : TCanvas; x1, y1, x2, y2, chR, clR, i : integer; txts : AnsiString; Lrect : TRect) : TRect;
begin
  with cv do begin
    // Test against Rect variables to set colour
    if (chR = i) and (clR = -1) then
      Brush.color := stateColor[bttn_on]
    else if (chR = i) and (clR = i) then
      Brush.color := stateColor[bttn_dwn]
    else
      Brush.color := stateColor[bttn_off];

    // Define Link Rectangle
    Lrect := Rect(x1, y1, x2, y2*(i+1));

    // Draw Rectangle
    Rectangle(Lrect);

    // Add text
    TextRect(Lrect, x1+5,  y1+5, txts);

    // Return co-ordinates of drawn rectangle as function result
    result := Lrect;
  end;
end;

// Canvas painting
procedure TOC_MenuPanel.Paint;
var
  y1, x2, y2, count : Integer;
begin
  inherited Paint;
  // Set length of array
  SetLength(MenuRects, fLinesText.Count);

  // Define Y1,Y2
  x2 := Width - Width div 20;
  y1 := Width div 20;

  with Canvas do begin
  // Draw outerRectangle outside of For loop
  Brush.color := clMenu;
  Rectangle(0, 0, Width, Height);

  // Loop for drawing menu items
  if fLinesText.Count = 0 then exit
  else
    for count := 0 to fLinesText.Count - 1 do begin
      // Define Y2
      y2 := TextHeight(fLinesText.strings[count])*2;

      // Cast rectangle draw function results to array for Mouse actions & draw rectangle
      MenuRects[count] := DrawRect(Canvas, 10, y1, x2, y2, chosenRect, clickedRect, count, fLinesText.strings[count], MenuRects[count]);

      // inc y1 for positioning the next box
      y1 := y1+y2;
    end;
  end;
end;
\$\endgroup\$
8
\$\begingroup\$

First, I'll go on about how the code looks, since it displays the most obvious issues.

You have redundant comments all over the place.

// Rectangle drawing function
Function DrawRect

The name DrawRect makes it clear enough that you're drawing a rectangle in there. No need for the comment.

Similarly for:

// Define Y2
y2 := TextHeight(fLinesText.strings[count])*2;

And in the case of:

// Define Y1,Y2
x2 := Width - Width div 20;
y1 := Width div 20;

it's not only redundant, but also misguiding, since you're not setting the value of Y2 anywhere in there.

Code should be self-documenting, comments should not explain what you're doing, at most they should explain why you're doing what it is that you're doing. Otherwise, they're just cluttering the file, requiring extra work to maintain and looking ugly.

You should work towards improving the names of your variables.

// Define Link Rectangle
Lrect := Rect(x1, y1, x2, y2*(i+1));

By calling Lrect LinkRectangle or at least LinkRect, you can also discard the comment above the assignment. LinkRect := Rect(...); makes it clear enough that you're setting up a link rectangle.


chR and clR are some awfully confusing names, and unless you look at what parameters are being passed to the function in an actual call, it's pretty hard to understand what they're supposed to do. Consider chosenRect, and clickedRect instead.


bttn_dwn could become button_down (if this is the style you decided to apply to what seem to be unsigned integer constants). txts could become text.


count is more often used to represent the number of items in a particular collection. For a loop counter, consider using a more common and appropriate name. In your particular case, there should be no issue to rename count to i.

And so on...

There is some inconsistency in the way you use the casing for your names.

Although Lrect is a parameter like all the others, it's being cased differently.


You chose to use Function while also using procedure. This sort of stands out, consider changing Function to function or procedure to Procedure, depending on how you want to be consistent. Personally, I prefer to not start with an uppercase character. Mainly because I don't start the rest of the keywords with one either, and I see you've also done that.

You should try to keep these things consistent throughout your code. Choose a way to name your things, and keep it throughout your project.

You could improve the way your code behaves.

if fLinesText.Count = 0 then exit
  else

could well become if fLinesText.Count > 0 then.

However, this too is redundant because for count := 0 to fLinesText.Count - 1 do begin will do nothing if Count is less than 1.


Consider extracting

if (chosenRect = i) and (clickedRect = -1) then
  Brush.color := stateColor[bttn_on]
else if (chosenRect = i) and (clickedRect = i) then
  Brush.color := stateColor[bttn_dwn]
else
  Brush.color := stateColor[bttn_off];

into a function that returns values of the type the stateColor elements have. Then call it from TOC_MenuPanel.Paint, and pass the result as parameter to DrawRect. This will ease the burden caused by the many parameters on DrawRect (you'll be able to get rid of chosenRect and clickedRect). It will also remove the responsibility of knowing that there are chosen and clicked rectangles, from DrawRect.


In the case of:

Lrect := Rect(x1, y1, x2, y2 * (i + 1));

You seem to be altering the rectangle's size based on the rectangle's index in the menu. There is no reason for DrawRect to even know that there's a menu and that the rectangle being drawn has an index in it. The raw value of y2 is not used anywhere else, so consider passing y2 * (i + 1) as parameter (from within MenuPanel.Paint), and getting rid of i as parameter. If you extract the color choosing bit in a separate function, and call it from Paint, doing this too should be trivial.


Consider wrapping up x1 x2 y1 y2 into some higher level abstraction (a record for example) or at least name them appropriately. (x, y, width, height) would go a long way towards making your code more understandable. Right now you have to carefully analyze how the code is written to find out if you're working exclusively with coordinates or if x2 and y2 somehow represent the width and height of the rectangle.

Using a higher level abstraction will also reduce the number of parameters you pass around, so I'd do that myself.


What methods could I use to increase the amount of encapsulation used in the drawing routine so I'm not defining variables in the Paint routine? The y2 variable in particular is irritating.

Follow the steps above to improve the way DrawRect works. As for the variables showing up in Paint, it depends on how far you want to go with your refactoring. In some form or another, the logic you used there has to exist. I do agree that it's probably better to take it out of the drawing routines.

Personally, I'd extract everything about positioning into some higher level structure that describes the whole layout. My drawing routines would receive the given layout and show it on the screen.

\$\endgroup\$
  • \$\begingroup\$ Now that, is a perfect answer. Thank you for the time spent looking through my code and providing a review. It's much appreciated. Its been a while since I looked at these routines but I'll go back over them later today. \$\endgroup\$ – Funk247 Nov 4 '14 at 9:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.