This code looks really repetitive. Any way to shorten it?

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; } 

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.

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; } 

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; } 

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.