Clearest Code Challenge: Jesse Ezel's answer inspires!
Jesse posted a comment with a unique approach: Make a class that's good at laying out buttons on the bottom-right of a form. I could definitely imagine adding a Help button, for example, which would make this generality helpful.
As for clarity, the downside is that the class itself is a bit more complexity to understand. On the upside, it's general in a way that lets you separate understanding of the implementation and the consumer. I think it's a win - you encapsulate the complexity & provide a pleasant programming model.
Here’s Jesse’s code:
void CornerAlignControls(int spacing, Control[] controls)
{
if (controls.Length == 0) return;
Size vertSpacing = new Size(spacing, 0);
Size horzSpacing = new Size(0, spacing);
Point lastLocation = new Point(parent.ClientSize - (vertSpacing + GetHeight(controls[0])));
for (int i = controls.Length - 1; i >= 0; i--)
{
lastLocation = (controls[i].Location = lastLocation - (horzSpacing + GetWidth(controls[i])));
}
}
Size GetWidth(Control c)
{
return new Size(c.Width, 0);
}
Size GetHeight(Control c)
{
return new Size(0, c.Height);
}
One of the really interesting things that happens in refactoring is that after reading the refactored code you see something else worth refactoring.
In this case, I think that my previous recommendation to prefer Size/Point object to separate X & Y values is no longer a good idea. The reason is that that Y value is held constant over the algorithm, while the X is manipulated repeatedly. Indicators (smells) include the GetWidth, GetHeight, vertSpacing, and horzSpacing items.
With that in mind, I’ve refactored Jesse’s code to deal with X & Y separately:
void CornerAlignControls(int spacing, Control[] controls)
{
int lastXLocation = Parent.ClientSize.Width;
int YLocation = Parent.ClientSize.Height - spacing;
for (int i = controls.Length - 1; i >= 0; i--)
{
lastXLocation -= controls[i].Width - spacing;
controls[i].Location = new Point(lastXLocation, YLocation);
}
}
I also get to remove the check for controls.Length, further simplifying the code. (if it turns out to be a perf hit because we call this routine in a tight loop with no controls, then we’ll add the check back in).
I’m not 100% sure it’s an improvement. Maybe if we keep the location pair in a Point, but manipulate the X directly:
void CornerAlignControls(int spacing, Control[] controls)
{
Point lastLocation = new Point(Parent.ClientSize.Width, Parent.ClientSize.Height - spacing);
for (int i = controls.Length - 1; i >= 0; i--)
{
lastLocation.X -= controls[i].Width - spacing;
controls[i].Location = lastLocation;
}
}
That does seem better to me.
One last smell: the reverse iteration. I find that when I write reverse iteration, I often get it wrong. It seems simple, but I still screw it up. Anyway, I like using foreach.
void CornerAlignControls(int spacing, Control[] controls)
{
List<Control> controlsReversedList = new List<Control>(controls).Reverse;
CornerAlignControls_Reversed(spacing, controlsReversedList.ToArray());
}
void CornerAlignControls_Reversed(int spacing, Control[] controls)
{
Point lastLocation = new Point(Parent.ClientSize.Width, Parent.ClientSize.Height - spacing);
foreach (Control control in controls)
{
lastLocation.X -= control.Width - spacing;
control.Location = lastLocation;
}
}
(I cheated a little: I used Generics that most of you don’t have access to. But you will.)
What do you think?
