Drawing and Painting a Menu Component

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, cho

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 prob

StackExchange Code Review Q#54704, answer score: 8