Entering specific models, brands, and prices into system

Question

Specifically the if/else statements trying to match model and brand. The code works, but it's ugly. Any suggestions to clean it up?

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace Cell_Phone_Test
{
        class CellPhone
    {
        // Fields
        private string _brand;      // Phone brand
        private string _model;      // Phone model
        private decimal _price;     // Phone price

        // Constructor
        public CellPhone()
        {
            _brand = "";
            _model = "";
            _price = 0m;
        }

        // Brand property
        public string Brand
        {
            get { return _brand; }
            set { _brand = value; }
        }

        // Model property
        public string Model
        {
            get { return _model; }
            set { _model = value; }
        }

        // Price property
        public decimal Price
        {
            get { return _price; }
            set { _price = value; }
        }
    }

    public partial class cellPhoneForm : Form
    {
        public cellPhoneForm()
        {
            InitializeComponent();
        }

        // The GetPhoneData method accepts a CellPhone object as an argument. It assigns the data entered by the user to the object's properties.
        private void GetPhoneData(CellPhone phone)
        {
            // Temporary variable to hold the price.
            decimal price;

            // Get the phone's brand. Check that it is a valid brand.
            if ((brandTextBox.Text.Contains("Google")) || (brandTextBox.Text.Contains("iPhone")) || (brandTextBox.Text.Contains("Samsung")))
            {                
                phone.Brand = brandTextBox.Text;
            }
            else
            {
                MessageBox.Show("Please enter a valid brand. (Google, iPhone, or Samsung)");
            }

            // Get the phone's model. Check that it is a valid model and matches brand.
            if ((modelTextBox.Text.Contains("Galaxy")) || (modelTextBox.Text.Contains("Note")) || (modelTextBox.Text.Contains("8")) || (modelTextBox.Text.Contains("X")) || (modelTextBox.Text.Contains("Pixel")))
            {
                if ((modelTextBox.Text.Contains("Galaxy")) || (modelTextBox.Text.Contains("Note")) == (brandTextBox.Text.Contains("Samsung"))) // 
                {
                    phone.Model = modelTextBox.Text;

                    if ((modelTextBox.Text.Contains("Pixel")) == (brandTextBox.Text.Contains("Google")))
                    {
                        phone.Model = modelTextBox.Text;

                        if ((modelTextBox.Text.Contains("8")) || (modelTextBox.Text.Contains("x")) == (brandTextBox.Text.Contains("iPhone")))
                        {
                            phone.Model = modelTextBox.Text;
                        }
                        else
                        {
                            MessageBox.Show("Please enter an iPhone model. (8 or X)");
                        }
                    }
                    else
                    {
                        MessageBox.Show("Please enter a Google model. (Pixel)");
                    }
                }
                else
                {
                    MessageBox.Show("Please enter a Samsung model. (Galaxy or Note)");
                }
            }
            else
            {
                MessageBox.Show("Please enter a valid model. (Galaxy, Note, 8, X, Pixel)");
            }
            // Get the phone's price. Check if it is within the range.
            if (decimal.TryParse(priceTextBox.Text, out price))
            {
                if (price >= 0.01m & price <= 2000.00m)
                {
                    phone.Price = price;
                }
                else
                {
                    MessageBox.Show("Please enter a price between $0.01 and $2000.00.");
                }
            }
            else
            {
                // Display an error message.
                MessageBox.Show("Invalid price");
            }
        }

        private void createObjectButton_Click(object sender, EventArgs e)
        {
            try
            {
                // Creat a CellPhone object.
                CellPhone myPhone = new CellPhone();

                // Get the phone data.
                GetPhoneData(myPhone);

                // Display the phone data.
                brandLabel.Text = myPhone.Brand;
                modelLabel.Text = myPhone.Model;
                priceLabel.Text = myPhone.Price.ToString("c");
            }
            catch
            {
                // Display an error message.
                MessageBox.Show("Invalid data was entered.");
            }
        }

        private void exitButton_Click(object sender, EventArgs e)
        {
            // Close the form.
            this.Close();
        }
    }
}
```

Answer 1

too many false arguments, for instance

if ((modelTextBox.Text.Contains("Pixel")) == (brandTextBox.Text.Contains("Google")))
{
    phone.Model = modelTextBox.Text;

    if ((modelTextBox.Text.Contains("8")) || (modelTextBox.Text.Contains("x")) == (brandTextBox.Text.Contains("iPhone")))
    {
        phone.Model = modelTextBox.Text;
    }
    else
    {
        MessageBox.Show("Please enter an iPhone model. (8 or X)");
    }
}
else
{
    MessageBox.Show("Please enter a Google model. (Pixel)");
}

if is it contains Pixel and Google, then how it could be possible to contain iPhone ?

your logic needs to be refactored and tested. The easiest way to refactor it is to invert the ifs. If you give a good look at the ifs, you'll find that phone.Model is assigned multiple times with modelTextBox.Text, which is redundant. it needs to be assigned once. So what you can do is to extract the validations and invert them to be outside and the compile would go throw them top to bottom, and if passed all, the bottom line would assign whatever value to phone.Model example :

// Get the phone's brand. Check that it is a valid brand.
if(!((brandTextBox.Text.Contains("Google")) || (brandTextBox.Text.Contains("iPhone")) || (brandTextBox.Text.Contains("Samsung"))))
{
    MessageBox.Show("Please enter a valid brand. (Google, iPhone, or Samsung)");
}

phone.Brand = brandTextBox.Text;

// Get the phone's model. Check that it is a valid model and matches brand.
if (!((modelTextBox.Text.Contains("Galaxy")) || (modelTextBox.Text.Contains("Note")) || (modelTextBox.Text.Contains("8")) || (modelTextBox.Text.Contains("X")) || (modelTextBox.Text.Contains("Pixel"))))
{
    MessageBox.Show("Please enter a valid model. (Galaxy, Note, 8, X, Pixel)");
}

if (!((modelTextBox.Text.Contains("Galaxy")) || (modelTextBox.Text.Contains("Note")) == (brandTextBox.Text.Contains("Samsung"))))
{
    MessageBox.Show("Please enter a Samsung model. (Galaxy or Note)");
}

if (!((modelTextBox.Text.Contains("Pixel")) == (brandTextBox.Text.Contains("Google"))))
{
    MessageBox.Show("Please enter a Google model. (Pixel)");
}

if (!((modelTextBox.Text.Contains("8")) || (modelTextBox.Text.Contains("x")) == (brandTextBox.Text.Contains("iPhone"))))
{
    MessageBox.Show("Please enter an iPhone model. (8 or X)");
}

phone.Model = modelTextBox.Text;

The real refactoring you need to do is to move the validations into separate methods, and keep the validations based on the brand. So, you now have three brands Google, iPhone, or Samsung it would be much cleaner if you implement three methods ValidateSuamsung(string model) and ValidateGoogle(string model) and ValidateGoogle(string model) then you can implement each validation inside its method, and then use a simple validation on the brand and execute its method like this :

// The GetPhoneData method accepts a CellPhone object as an argument. It assigns the data entered by the user to the object's properties.

    private void GetPhoneData(CellPhone phone)
    {
        switch(brandTextBox.Text)
        {
            case "Google":
                ValidateGoogle(modelTextBox.Text);
                break;
            case "Samsung":
                ValidateSamsung(modelTextBox.Text);
                break;
            case "iPhone":
                ValidateiPhone(modelTextBox.Text);
                break;
            default: 
                MessageBox.Show("Please enter a valid brand. (Google, iPhone, or Samsung)");
                break;          
        }

        phone.Brand = brandTextBox.Text;
    }

Also, since you have multiple Contains, you could do something like :

// Get the phone's model. Check that it is a valid model and matches brand.

var model = new string[] { "Galaxy", "Note", "8", "X", "Pixel" };

if(!model.Contains(modelTextBox.Text))
{
    MessageBox.Show("Please enter a valid model. (Galaxy, Note, 8, X, Pixel)");
}

if you move the validations based on the brand like I suggested, I beleive that you'll make a simpler validation and shorter as well, as you'll validate based on brand, which will make things more specific and straight forward.

Answer 2

A few things to consider:

Since you're not doing anything special with the getters and setters of your properties, I would suggest using the simple version(string Brand { get; set; }).

To simplify your validation, I would suggest a class that holds a collection of CellPhone's. Leveraging the underlying collection would allow you to fill a combobox with choices. Choose the brand, then fill the next combobox with the appropriate models. Since the choices come from the collection they're automatically correct.

If you make the underlying collection a Dictionary<string,Dictionary<string,CellPhone>> You can get near constant time lookups to search for a particular brand and model.

Such a class could look like this:

private class CellPhones
{
    public class CellPhone
    {
        public string Brand { get; set; }
        public string Model { get; set; }
        public decimal Price { get; set; }

        public CellPhone() { }
        public CellPhone(string brand, string model, decimal price)
        {
            Brand = brand;
            Model = model;
            Price = price;
        }

    }
    Dictionary<string,Dictionary<string,CellPhone>> phones { get; set; }
    public CellPhones()
    {
        phones = new Dictionary<string, Dictionary<string, CellPhone>>();
    }
    public void AddUpdatePhone(string brand, string model, decimal price)
    {
        CellPhone newPhone = new CellPhone(brand, model, price);
        if(!phones.ContainsKey(brand))
        {
            phones.Add(brand, new Dictionary<string, CellPhone>());
        }
        if(!phones[brand].ContainsKey(model))
        {
            phones[brand].Add(model, newPhone);
            return;
        }
        if(phones[brand][model].Price != price)
        {
            phones[brand][model].Price = price;
        }
    }
    public List<string> GetBrands()
    {
        return phones.Keys.ToList();
    }
    public List<string> GetModels(string brand)
    {
        return phones[brand].Keys.ToList();
    }
}