5
\$\begingroup\$

I have something like this in my program:

    private void tspBrush_Click(object sender, EventArgs e)
    {
        currentTool = new Brush(tileLayers);

        UncheckToolstripButtons();

        tspBrush.Checked = true;
    }

    private void tspBucket_Click(object sender, EventArgs e)
    {
        currentTool = new Bucket(tileLayers);

        UncheckToolstripButtons();

        tspBucket.Checked = true;
    }

    private void tspCut_Click(object sender, EventArgs e)
    {
        currentTool = new Cut(tileLayers);

        UncheckToolstripButtons();

        tspCut.Checked = true;
    }

As I increase the amount of tools this list will only get longer. Is there something I could do to shorten it?

\$\endgroup\$
11
\$\begingroup\$

one way of shortening might be:

  • having a HashMap consisting of pairs of objects (sender) and Tools. This way you could look up the appropriate Tool for each sender in one single method like this:

    HashMap<object, Tool> hashMap = new HashMap<object, Tool>( );
    
    hashMap.put(sender1, new Brush(tileLayers));
    
    // ...add the rest of the Tools
    private void clickHandler(object sender, EventArgs e)
    {
        currentTool = hashMap.get(sender);
    
        UncheckToolstripButtons( );
        tspBrush.Checked = true;
    }
    
  • If tspXxxx cannot be retrieved from the sender directly, use another HashMap to map senders to tspXxxx objects.

\$\endgroup\$
  • \$\begingroup\$ +1: Can't really tell if the OP is C# or Java, but it looks like a HashMap is the same as a Dictionary<TKey, TValue> ...??? \$\endgroup\$ – IAbstract Aug 8 '11 at 1:51
  • 2
    \$\begingroup\$ Just had a look at the C# class Dictionary<TKey, TValue> and it seems to implement the same functionality as HashMap in Java, yeah. And since the parameter type 'object' is written in lowercase it's C#, sorry for my assuming it was Java. But the solution does apply though. \$\endgroup\$ – Jan Bolting Aug 8 '11 at 10:02
1
\$\begingroup\$

I'm calling the common item "Tool" but I'm sure you have another name for it...

private void tspBrush_Click(object sender, EventArgs e)
    {
        setTool(tspBrush);
    }

private void tspBucket_Click(object sender, EventArgs e)
    {
        setTool(tspBucket);
    }

private void tspCut_Click(object sender, EventArgs e)
    {
        setTool(tspCut);
    }

private void setTool(Tool tool)
    {
        currentTool = new Brush(tileLayers);

        UncheckToolstripButtons();

        tool.Checked = true;
    }
\$\endgroup\$
  • \$\begingroup\$ I'm not sure how this works? setTool just creates a new brush for all of them and Tool does not have a Checked property. \$\endgroup\$ – John Smith Aug 7 '11 at 16:55
  • \$\begingroup\$ I knew I shouldn't have called it "Tool". The thing is, whatever class tspCut, tspBrush, and tspBucket are - that is the correct class to be the parameter to setTool. Maybe I should have called it Tsp. \$\endgroup\$ – Carl Manaster Aug 8 '11 at 14:41
1
\$\begingroup\$

It's difficult to say something definit, because a lot of information is missing. But here is my approach:
1. Create type of the tool:

    private enum ToolType
    {
        Brush,
        Bucket,
        Cut
    }

2. Create inner helper class:

    // Inner class that represents a tool in this particular form (control, page)
    private class UITool
    {
        public MyToolType Type { get; set; }
        public Tool Tool { get; set; } //'Tool' is the parent of Brush, Bucket, Cut
        public CheckBox ToolCheckBox { get; set; } //'CheckBox' a base class for tspBrush, tspBucket, tspCut

        /*If tspBrush, tspBucket, tspCut don't belong to the same base class with Checked property,
        then don't use ToolCheckBox property and use SetToolCollback instead.
        public Action SetToolCollback { get; set; } */
    }

3. Create private property

private List<UITool> _uiTools = new List<UITool>();

4. Init it somewhere

         _uiTools.Add(new UITool()
        {
            Type = ToolType.Brush,
            Tool = new Brush(tileLayers),
            ToolCheckBox = tspBrush
        });

or if you use SetToolCollback property:

            _uiTools.Add(new UITool()
                         {
                             Type = ToolType.Brush,
                             Tool = new Brush(tileLayers),
                             SetToolCollback = () => tspBrush.Checked = true
        });

5. Create method:

    private void SelectTool(ToolType toolType)
    {
        var utTool = _uiTools.Select(i => i.Type == toolType).Single();
        currentTool = utTool.Tool;
        UncheckToolstripButtons();
        utTool.ToolCheckBox.Checked = true;           
        //If using SetToolCollback
        //utTool.SetToolCollback();
    }

6. Use this method:

private void tspBrush_Click(object sender, EventArgs e)
{
    SelectTool(ToolType.Brush);
}

As a variant, you can replace ToolType with the type of 'sender' and use common event handler for all tools like Jan Bolting have suggested.

\$\endgroup\$
  • \$\begingroup\$ using the enumerator seems to make this much more difficult than it should be \$\endgroup\$ – IAbstract Aug 8 '11 at 1:46
  • 1
    \$\begingroup\$ @IAbstract - It may seems difficult, but in the end it allows you to add new tools more easily. Actually, this approach is like Jan Bolting's approach, the only difference is in the helper class UITool. So, instead of getting a concrete tool from the HasMap and getting 'tspXXX' from another HasMap, you get a UITool with all nessesary data. \$\endgroup\$ – Ivan Aug 8 '11 at 7:33
1
\$\begingroup\$

Apologise errors in the pseudo-code please - I don't have a C# compiler on this machine. If it is possible to make an interface "Checkable" that tspCut, tspBrush, tspBucket and their future siblings must obey, you could use a Factory.

private void tspCut_Click(object sender, EventArgs e)  
{  
    createTool( Click.GetType(), tspCut );  
}  

private void createTool( ToolType tt, Checkable tsp )  
{  
    currentTool = ToolFactory.create( tt );  
    UncheckToolstripButtons( );  
    tsp.Checked = true;  
} 
\$\endgroup\$
0
\$\begingroup\$

Jan's answer converted to C# (assuming tspBrush is a ToolStripMenuItem):

    Dictionary<ToolStripMenuItem, Tool> tools = new Dictionary<ToolStripMenuItem, Tool>();

    // for each tool:
    tools.Add(tspBrush, new Brush(tileLayers));
    tspBrush.Click += toolMenuItem_Click;


    private void toolMenuItem_Click(object sender, EventArgs e)
    {
        var menuItem = (ToolStripMenuItem)sender;

        currentTool = tools[menuItem];
        UncheckToolstripButtons();
        menuItem.Checked = true;
    }
\$\endgroup\$
0
\$\begingroup\$

A more attractive solution IMHO would be to associate your tool items with the buttons themselves using the Tag property. That way you could have one single click event handler for all your tool buttons. Remember, the sender that is passed to your handler is the control that initiated the event so you don't need to wire it to a specific control, but more general to be used for all controls. Furthermore, using Lazy<T> wrapping your tools to ensure that you only instantiate one instance of them only when you need it (if and when you need it). This would go smoothly if all your tools implemented a certain interface or derived from the same class.

// associating the buttons
tspBrush.Tag = new Lazy<ITool>(() => new Brush(tileLayers));
tspBucket.Tag = new Lazy<ITool>(() => new Bucket(tileLayers));
tspCut.Tag = new Lazy<ITool>(() => new Cut(tileLayers));

// THE click handler
private void ToolButton_Click(object sender, EventArgs e)
{
    var button = sender as ToolStripButton;
    if (button == null) return;
    var lazy = button.Tag as Lazy<ITool>;
    if (lazy == null) return;

    currentTool = lazy.Value;
    UncheckToolstripButtons();
    button.Checked = true;
}

If you need to recreate the tools every time, then rather than using Lazy<T> to wrap it, you could use the Func<ITool> instead to create the tool every time. Then it would be:

// associating the buttons
tspBrush.Tag = new Func<ITool>(() => new Brush(tileLayers));
tspBucket.Tag = new Func<ITool>(() => new Bucket(tileLayers));
tspCut.Tag = new Func<ITool>(() => new Cut(tileLayers));

// THE click handler
private void ToolButton_Click(object sender, EventArgs e)
{
    var button = sender as ToolStripButton;
    if (button == null) return;
    var factory = button.Tag as Func<ITool>;
    if (factory == null) return;

    currentTool = factory();
    UncheckToolstripButtons();
    button.Checked = true;
}

However if you needed to go this route, I would suggest adding methods to update the tools instead.

\$\endgroup\$

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.