Repetitive code driving me crazy!

Question

Ok, So first I must say that everything I know about coding I have learned on my own in my spare time so bear with me if my code is primitive, but please, I am open to any comments to make me better...

Anyway, as for my question. I have an application I am building in C# for .NET Compact Framework (for an HP iPaq) the program's purpose is to act similarly to a Restaurant POS terminal to "ring up" food orders. it has gotten to the point where some of my code is the same line copied god even knows how many times with only a numerical difference between them. here is an example:

    private void button9_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[0], prices[0]);
    }

    private void button14_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[1], prices[1]);
    }

    private void button5_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[2], prices[2]);
    }


    private void button10_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[3], prices[3]);
    }

    private void button13_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[4], prices[4]);
    }

    private void button4_Click(object sender, EventArgs e)
    {
        AddItem(buttonNames[5], prices[5]);
    }

or

                button9.Text = buttonNames[0];
                button14.Text = buttonNames[1];
                button5.Text = buttonNames[2];
                button10.Text = buttonNames[3];
                button13.Text = buttonNames[4];
                button4.Text = buttonNames[5];
                button11.Text = buttonNames[6];
                button15.Text = buttonNames[7];
                button7.Text = buttonNames[8];
                button12.Text = buttonNames[9];
                button16.Text = buttonNames[10];
                button8.Text = buttonNames[11];

I KNOW there are easier ways to do a lot of the code I have written, I just don't know how to do it.

Answer 1

Even though the difference is minimal, there IS a difference. So you'll need to make a minimal distinction, depending on the button that was pressed.

What you could do is create a general event for all the buttons and in that event you'll determine the index to be passed to the AddItem() method. The code might look like this:

private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;

    switch(b.Name)
    {
        case "button9" : AddItem(buttonNames[0], prices[0]); break;
        case "button14" : AddItem(buttonNames[1], prices[1]); break;
        case "button5" : AddItem(buttonNames[2], prices[2]); break;
        //other buttons...
    }
}

Another solution is to pre-assign the index to the Tag property of your button:

button9.Text = buttonNames[0];
button9.Tag = 0;
//Same for other buttons

This way you can also use a general method like before and use the tag to call the AddItem() method. Like this:

private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;
    int index = Convert.ToInt32(b.Tag);
    AddItem(buttonNames[index], prices[index]);
}

Lastly, if you name your buttons in a different way, it will become even more easy to do this. Place the index to be used in the name of the button and get it from the name in the general event. Example:

//Assignment of the buttons:
button0.Text = buttonNames[0];
button1.Text = buttonNames[1];
button2.Text = buttonNames[2];

//Event:
private void ButtonClick(object sender, EventArgs e)
{
    Button b = (Button)sender;
    int index = Convert.ToInt32(b.Name.Replace("button", ""));
    AddItem(buttonNames[index], prices[index]);
}

Hope this helps!

Answer 2

Clear your namings

button9.Text = buttonNames[0]; should be button0.Text = buttonNames[0]; This is also applies to your prices array.

One event handler method

Create one event handler which is subscribed on every buttons' OnClick and this method should parse the sender name and look for the button's index. If you can read the index number (from the above example the 0) then you can say:

AddItem(buttonNames[parsedIndex], prices[parsedIndex]);

Answer 3

If you can, I think the best solution would be to create those buttons from code behind (which is BTW much easier to do in WPF thanks to panels, binding and templates). Something like:

for (int i = 0; i < 6; i++)
{
    var button = new Button();
    // set the position of the button based on i here
    int iCopy = i; // to make closure work correctly
    button.Click += (s, e) => AddItem(buttonNames[iCopy], prices[iCopy]);
    this.Controls.Add(button); // assumes this is a Form
}

Also, having two arrays with synchronized indexes is a code smell. You should probably have just one array which contains objects with properties ButtonName and Price. Then you could do just AddItem(items[iCopy]).

Answer 4

Here's how you'd do it by using a map. This has several advantage including the ability to support additional buttons and items in a single line of code. The only seemingly repeating code is where you specify the map.

Here we use an object to represent a food item instead of 2 arrays

public class FoodItem
{
    public String Name { get; set; }
    public double Price { get; set; }
    public override string ToString()
    {
        return Name + "@" + Price;
    }
}

Then we use the fooditem in a map with buttons

    //Some setup in order to get our buttons all situated. 
    private void Form1_Load(object sender, EventArgs e)
    {
        FillButtonMap(); //First load the button map
        SetupButtons(); //Then register the click events to the proper additem call
    }

    public Dictionary<Button, FoodItem> ButtonMap = new Dictionary<Button, FoodItem>();
    //Here is where you add more buttons.
    void FillButtonMap()
    {
        ButtonMap.Add(button1, new FoodItem() { Name = "Taco", Price = 1.0 });
        ButtonMap.Add(button2, new FoodItem() { Name = "Burrito", Price = 2.0 });
        ButtonMap.Add(button3, new FoodItem() { Name = "Tostada", Price = 3.5 });
    }

    //This part sets the button text properly and registers the click event 
    //of the button to add the item 
    void SetupButtons()
    {
        foreach (var button in ButtonMap.Keys)
        {
            button.Text = ButtonMap[button].Name; //Set the button text
            var mappedFoodItem = ButtonMap[button]; //get the food item for the button 
            button.Click += (s, o) => { AddItem(mappedFoodItem); }; //Set the click event
        }
    }

    //Change AddItem to accept a FoodItem instead of String,Number
    void AddItem(FoodItem item)
    { //  Your Code Here...      }

To add Salsa you simply add this line in FillButtonMap...

    ButtonMap.Add(button4, new FoodItem() { Name = "Salsa", Price = 0.25 });

To make this more flexible you'd actually move the FillButtonMap into an external file without having to statically connect them to buttons so you could add more menu items simply by adding them to the text file (or XML, or JSON, or whatever format you want) but that's another topic as well.

Answer 5

All good answers. I'd like to add my two cents to @deepee1's answer. If you keep the FoodItem class he provides (with a constructor), you can create a list of food items and then create everything else with completely DRY code. You can even easily move the definitions out of the code. Not to mention creating n different UI's with the same "logic".

Here's what I'd do: (Forgive any mistakes, long time since WinForms :) )

private List<FoodItem> foods = new[] {
    new FoodItem("Taco", 1.0),
    new FoodItem("Burrito", 2.0),
    new FoodItem("Fajita", 1.5)
};
private const int buttonMargin = 5;

protected void Form_Load(...)
{
    CreateButtons();
}

private void CreateButtons()
{
    for(var i = 0; i<foods.Length; i++)
        AddButton(foods[i], i);
}

private void AddButton(FoodItem food, int index)
{
    var btn = new Button();
    btn.Id = "Button_" + i;
    btn.Tag = food;
    btn.Click += FoodClicked;
    btn.Top = btn.Height*index + buttonMargin*index;
    // Whatever initialization code you have
    Controls.Add(btn);
}

private void FoodClicked(object sender, EventArgs e)
{
    var button = (Button)sender;
    var food = (food)button.Tag;
    // Do whatever with the food..
}

I just re-read some comments and realize you need paging too. Add a private int page, and a private int pagesize, then add the multiplication of those to foods[i] in CreateButtons, and swap foods.Length with pagesize. You can have as big an array as you want then.