11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You could organize the buttons by keeping them in an array or list and then create a generic click event. If you have certain data associated with the buttons, such as prices and names, you could create a class which has properties that could store this information instead of keeping it separate. Buttons[0].name, Buttons[0].price, Buttons[0].buttonControl, etc. \$\endgroup\$ – mowwwalker Jun 13 '13 at 21:28
15
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ Thanks for this answer! this has many options for me to work with, but now I have another question. another thing I am doing is re-using the same buttons to "page" so say clicking one button will swap EVERYTHING about the button, for example on the when I page from one page to the next, the text on the button must change to the text from a different position in buttonNames, and (currently) the handler needs to change to accept that the new index needs to change... is there a way to simplify that? Thanks again for the help! \$\endgroup\$ – Michael Jun 13 '13 at 17:22
  • \$\begingroup\$ and one more thing i should mention is the fact that the reason I am changing handlers (currently) as opposed to something like AddItem(butonNames[(index+(page*12)), prices[(index+(page*12))]); is because some buttons DO need completely different handlers. or should I just overload AddItems() so that it can somehow accept the different parameters? \$\endgroup\$ – Michael Jun 13 '13 at 17:28
6
\$\begingroup\$

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]);
\$\endgroup\$
  • \$\begingroup\$ Yes, I agree about the buttons being named better, however, I keep moving the buttons on the form (using visual studio) and it is (currently) easier to get the basics worked out in terms of having it work, then when it is finally the way I want it then fixing that up and doing a "re-write" that is easier to follow. \$\endgroup\$ – Michael Jun 13 '13 at 17:13
3
\$\begingroup\$

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]).

\$\endgroup\$
  • \$\begingroup\$ adding the buttons via code is not something I am particularly good at right now, but yes I realize that this is a possible easier solution. I have tried to add some buttons via code and the problems I encountered were 1) These buttons are located on a user control which is on a form. and my buttons always end up in the wrong place! and 2) I don't know (until the array is populated) how many buttons will be used, although never more than 36. the best way that i know how to have 12 buttons and swap the handlers. although if you know of a trick that i could use I am ALWAYS open to learn. Thanks! \$\endgroup\$ – Michael Jun 13 '13 at 17:40
  • \$\begingroup\$ @Michael Can't you just create the buttons after the array is populated? \$\endgroup\$ – svick Jun 13 '13 at 18:41
  • \$\begingroup\$ Yes, which solves half of the issue, for example: I would have to have it create the first 12 buttons then stop. then if the user pages to the next one over it needs to then remove the old buttons and replace them with buttons using the info from the middle 12 indexes, and then if they page back it needs to start at the beginning again. and the HP Ipaq is easily slowed, so wont having to redraw the buttons after every click basically slow it down immensely? \$\endgroup\$ – Michael Jun 13 '13 at 18:57
  • \$\begingroup\$ Oh, I guess I didnt mention that only 12 buttons fit per page... guess thats important \$\endgroup\$ – Michael Jun 13 '13 at 19:14
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I REALLY want to do this via XML. I feel it would potentially be a ton easier after the initial setup. BUT it seems that the HP Ipaq using .NET CF is a tad bit slow at processing XML files and performance seems to drop significantly when using them... any tips on XML for .NET CF that might help performance? \$\endgroup\$ – Michael Jun 14 '13 at 4:31
  • \$\begingroup\$ Just load the xml once during load of your application then keep the data in memory. The associations you have will likely take up very little memory. I assume we're talking <100 menu items which likely means <10kb. \$\endgroup\$ – deepee1 Jun 14 '13 at 13:43
  • \$\begingroup\$ Yeah, I guess if loading it at the start a little delay isn't too bad... Will have to toy with it :) Thanks! \$\endgroup\$ – Michael Jun 14 '13 at 14:32
0
\$\begingroup\$

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.

\$\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.