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?


 


 

Comments (5)

  1. Thomas Eyde says:

    Very good!

    I replaced my two statics on my ControlPositioner with the one below to match the place-all-in-corner solution. I am still using Jay’s original algorithm:

    public static void PositionInLowerRightCorner(Control[] controls, Form f, int margin)

    {

    Control relative = null;

    foreach (Control c in controls)

    {

    ControlPositioner cp = new ControlPositioner(c, margin);

    if (relative == null)

    {

    cp.PositionInLowerRightCornerOf(f);

    }

    else

    {

    cp.PositionToLeftOf(relative);

    }

    relative = c;

    }

    }

  2. BillT says:

    This is fun!

    So now we have CornerAlignControls, which is a LayoutManager or ControlLayoutTool.

    What could we do to:

    (1) make a more general ControlLayoutTool, or

    (2) specify the layout rules, or

    (3) extend all these ideas?

  3. David Kean says:

    I think you’ll find you need to change this line:

    lastLocation.X -= control.Width – spacing;

    to

    lastLocation.X -= control.Width + spacing;

    You will also need to think about Right-To-Left and AutoScale (the spacing should be based on AutoScaleBaseSize and the auto scale size of the new font…

    Then it starts to get complicated…

  4. Steve says:

    btw, Jaybaz, how are you formatting your code in your posts. It looks really nicde.

    Would you care to share? Thanks!

  5. It’s really cool to see where this is going. Not one of us could have come this far alone. The group dynamic is amazing.

    BillT: if you want to carry this further, it might be tough to do in the comments of my blog. I’d suggest using your own blog.