Pizza form project

Question

I've just recently started C# and I was wondering if there's anything I can improve on.

My project is a pizza form, where people can order pizza and the price gets updated, depending on whether they want delivery or not.

Here's a picture of the GUI:

enter image description here

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
namespace PizzaMiniProject {
 public partial class Form1 : Form {
 public Form1() {
 InitializeComponent();
 MaximizeBox = false;
 }
 private void btnAdd_Click(object sender, EventArgs e) {
 bool error = false;
 if (lbPizzaSelection.SelectedItem != null) {
 if (txtQuantity.Text != "") {
 lbQuantity.Items.Add(txtQuantity.Text);
 int cost = 0;
 if (lbPizzaSelection.SelectedItem == "Neapolitan") {
 cost = Convert.ToInt32(txtQuantity.Text) * 50;
 lbAmount.Items.Add(cost);
 }
 if (lbPizzaSelection.SelectedItem == "Margherita") {
 cost = Convert.ToInt32(txtQuantity.Text) * 60;
 lbAmount.Items.Add(cost);
 }
 if (lbPizzaSelection.SelectedItem == "Pepperoni") {
 cost = Convert.ToInt32(txtQuantity.Text) * 80;
 lbAmount.Items.Add(cost);
 }
 if (lbPizzaSelection.SelectedItem == "Lazio") {
 cost = Convert.ToInt32(txtQuantity.Text) * 70;
 lbAmount.Items.Add(cost);
 }
 if (lbPizzaSelection.SelectedItem == "Zucchini") {
 cost = Convert.ToInt32(txtQuantity.Text) * 90;
 lbAmount.Items.Add(cost);
 }
 txtQuantity.Focus();
 txtQuantity.Clear();
 } else {
 error = true;
 MessageBox.Show("Please enter a valid Quantity", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 lblAmount.Focus();
 }
 if (error == false) {
 lbOrdered.Items.Add(lbPizzaSelection.SelectedItem);
 lbPizzaSelection.Items.Remove(lbPizzaSelection.SelectedItem);
 }
 } else {
 MessageBox.Show("No Pizza Selected", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 }
 }
 private void txtQuantity_KeyPress(object sender, KeyPressEventArgs e) {
 char ch = e.KeyChar;
 if (!Char.IsDigit(ch) && ch != 8) //method makes it so all digits[numbers] can be clicked expect the backwards which is 8.
 {
 e.Handled = true;
 MessageBox.Show("Quantity must be a number!", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 }
 }
 private void btnReset_Click(object sender, EventArgs e) {
 if (checkDelivery.Checked) {
 checkDelivery.Checked = false;
 }
 lbOrdered.Items.Clear();
 lbAmount.Items.Clear();
 lbQuantity.Items.Clear();
 lblAmount.Text = "";
 lblOrder.Text = "";
 MessageBox.Show("Form Reset.", "Success", MessageBoxButtons.OK, MessageBoxIcon.Information);
 }
 private void btnOrder_Click(object sender, EventArgs e) {
 if (lbOrdered.Items.Count > 0) {
 int sum = 0;
 for (int i = 0; i < lbAmount.Items.Count; i++) {//using a dummy variable i to store
 sum += Convert.ToInt32(lbAmount.Items[i].ToString());//convert to int or double in order to be mathimtical
 }//add to the sum instead of keeps changing in order to get total value.
 lblAmount.Text = Convert.ToString(sum);//once done re-convert to string t be used.
 double[] currencies = { 1, 3.67, 5.12 };
 int currency = 0;
 if (currencyUAE.Checked) {
 currency = 0;
 }
 if (currencyUS.Checked) {
 currency = 1;
 }
 if (currencyEuro.Checked) {
 currency = 2;
 }
 double price = int.Parse(lblAmount.Text) / currencies[currency];
 lblAmount.Text = string.Format("{0:#.##}", price);
 if (checkDelivery.Checked) {
 double totalPrice = double.Parse(lblAmount.Text) + (double.Parse(lblAmount.Text) * 0.05);
 lblOrder.Text = string.Format("{0:#.##}", totalPrice);
 MessageBox.Show("Have a nice meal! - No Express Delivery!");
 } else {
 double totalPrice = double.Parse(lblAmount.Text);
 lblOrder.Text = string.Format("{0:#.##}", totalPrice);
 MessageBox.Show("Have a nice meal!");
 }
 } else {
 MessageBox.Show("Place an order first!", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 }
 }
 }
}

Answer 1

Constructor

public Form1() {
 InitializeComponent();
 MaximizeBox = false;
}

This is your form's constructor - the InitializeComponents method is where everything you've set up in the designer is happening; go ahead, take a look - you'll see this somewhere:

btnAdd.Click += new EventHandler(this.btnAdd_Click);

This is of course the long-winded way of registering an event - this accomplishes the same thing:

btnAdd.Click += btnAdd_Click;

The point is, if you're using the designer, this InitializeComponents method is where one would expect the ...initialization of components to take place.

This InitializeComponents method should include assigning MaximizeBox to false...

That said, I like that your constructor does absolutely nothing but initializing components, even if that's in two places. I've seen much worse beginner code! :)

---

Separation of Concerns

The entire application's logic resides in the form's code-behind - you've essentially written a SmartUI, which is good for a prototype, or for fiddling around and learning how things work, but let's see how we can separate the concerns a bit.

The UI is stealing the show here.. You can start with the designer and drawing your form as you want it, and then before you type anything in the form's code-behind, create a class that contains everything your form needs:

• The menu, including the name and the price (and a witty description?) of each MenuItem.
• An Order, including:
  • A list of OrderItems, each item including:
    • a MenuItem (with its description and price)
    • a Quantity
  • A SupportedCurrency
  • A special flag when the order IsWithExpressDelivery

Then you'll need another class to act upon these objects, and expose methods and properties that your view can use, with a little help from the code-behind.

Write your code so that if you were asked to make it be able to create an order coming from an online order form on the restaurant's website, you wouldn't have to rewrite the entire program, and all you'd have to worry about is "ok, how do I make it look?".

When you're done, your btnAdd_Click method might look as simple as this:

private void btnAdd_Click(object sender, EventArgs e)
{
 _viewModel.AddOrderItem();
}
private void ViewModel_CanAddOrderItemChanged(object sender, IsEnabledChangedEventArgs e)
{
 btnAdd.Enabled = e.IsEnabled;
}

This relies on an event-driven pattern, but I encourage you to read up on data binding and see what it can do for you in winforms, and also to look into wpf, which revolves around data binding, which eliminates a lot of the "unimportant stuff" from the handler's code (updating the UI), and automates keeping the UI in sync with the data you have in your objects.

• MrKhoori: Thank you for your help! I'll definitely try to understand more about what you did and read about data binding!

Answer 2

Your btnAdd_Click routine could use some clean up. Instead of hardcoding all of the values, set yourself up an enumeration to use. It will make your code easier to read, and the values that need to be maintained easier to find later.

Private enum CostFactor {
 Neapolitan = 50, 
 Margherita = 60,
 Lazio = 70,
 Pepperoni = 80,
 Zucchini = 90
}

Then replace all of those if statements with a switch. I got rid of your cost variable entirely and we'll only write the line of code that sets your label once.

private void btnAdd_Click(object sender, EventArgs e) {
 bool error = false;
 if (lbPizzaSelection.SelectedItem != null) {
 if (txtQuantity.Text != "") {
 lbQuantity.Items.Add(txtQuantity.Text);
 int selectedCostFactor = 0;
 switch (lbPizzaSelection.SelectedItem.ToString()) {
 Case "Neapolitan"
 selectedCostFactor = (int)CostFactor.Neapolitan;
 break;
 Case "Margherita"
 selectedCostFactor = (int)CostFactor.Margherita;
 break;
 Case "Lazio"
 selectedCostFactor = (int)CostFactor.Lazio;
 break;
 Case "Pepperoni"
 selectedCostFactor = (int)CostFactor.Pepperoni;
 break;
 Case "Zucchini"
 selectedCostFactor = (int)CostFactor.Zucchini;
 break;
 }
 lbAmount.Items.Add(Convert.ToInt32(txtQuantity.Text) * selectedCostFactor);
 txtQuantity.Focus();
 txtQuantity.Clear();
 } 

Your currency check should be an else if, if for no other reason than you won't have to check all of the values every time.

if (currencyUAE.Checked) {
 currency = 0;
}
else if (currencyUS.Checked) {
 currency = 1;
}
else if (currencyEuro.Checked) {
 currency = 2;
}

Oh, and currency mathematics should use decimal, not double. THere's less chance of a rounding error occurring with a decimal.

• MrKhoori: I tried doing the enum way.. got this i.imgur.com/Zj0dbLd.png after adding a null check from what i understood.. i got this.. i.imgur.com/tTAWWln.png
• RubberDuck: Sorry about that. I rarely use C#. Take a look at this for the switch issue. I'm actually a little concerned we've discovered a potential bug.
• Mason11987: Regarding the witch on .SelectedItem... but that returns an object, you want listbox.SelectedItem.ToString() in order to do what they were suggesting. Also, one way to fix the second error you commented about is to add (int) in front of each of the Costfactor.Entry items. Those are enums and the compiler is looking for an int, so tell it that a conversion exists by being explicit. For example: selectedCostFactor = (int)CostFactor.Neapolitan

Answer 3

Adding to RubberDuck's answer, I would use a Dictionary<string, CostFactor> instead of a switch-case, which I believe is a more scalable approach. Therein, you would set the values first and then simply perform a lookup:

dictionary[lbPizzaSelection.SelectedItem.ToString()]

I would definitely not rely on array indices with the currencies and instead inline the values, so you could write (without a map):

double currency = 0.0;
if (currencyUAE.Checked) {
 currency = 1; 
} else if (currencyUS.Checked) {
 currency = 3.67;
} else if (currencyEuro.Checked) {
 currency = 5.12;
}

Again, you should be able to restructure the non-scalable if-else to use a Dictionary.

• chillworld: Welcome to code review, I'm glad that you couldn't commented this answer because it fit as an answer.