Product Manager managing product listings and prices

Question

For school we have to develop a product manager. The basic task is to add new products. A product has a name and a price.

I know the one or other class is currently pretty empty but its planned to add more methods.

The code is developed in MVC-format and comments plus texts are in German.

I would like to know what's good and what's bad (also why) and how I can improve the code in design, structure, variable/method naming, performance.

Program.cs

using System;
using Ferienaufgabe_2.Controller;

namespace Ferienaufgabe_2
{
    static class Program
    {
        /// <summary>
        /// Der Haupteinstiegspunkt für die Anwendung.
        /// </summary>
        [STAThread]
        static void Main()
        {
           ProductList productList = new ProductList();
           productList.RenderView();
        }
    }
}

Controller/ProductList.cs

using System.Windows.Forms;

namespace Ferienaufgabe_2.Controller
{
    class ProductList
    {
        public void RenderView()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            Application.Run(new View.ProductList());
        }
    }
}

Model/Product.cs

namespace Ferienaufgabe_2.Model
{
class Product
{
    private string productname;
    private decimal price;

    public Product(string productname, decimal price)
    {
        this.Productname = productname;
        this.Price = price;
    }

    private string Productname
    {
        set
        {
            this.productname = value;
        }
        get
        {
            return this.productname;
        }
    }

    private decimal Price
    {
        set
        {
            this.price = value;
        }
        get
        {
            return this.price;
        }
    }
}
}

Utility/Validate.cs

using System.Text.RegularExpressions;

namespace Ferienaufgabe_2.Utility
{
class Validate
{
    public bool IsValidPriceFormat(string value)
    {
        string strRegex = @"^[0-9]{1,}([\,]{0,1}[0-9]{1,2}){0,1}$";
        Regex myRegex = new Regex(strRegex, RegexOptions.None);
        Match myMatch = myRegex.Match(value);
        if (myMatch.Success)
        {
            return true;
        }
        return false;
    }
}
}

View/ProductList.cs

Build with MS VS 2015 RC - form designer

using System;
using System.Drawing;
using System.Windows.Forms;

namespace Ferienaufgabe_2.View
{
public partial class ProductList : Form
{
    private string defaultTextAddProductTextBox;
    private string messageMissingProductname;

    public ProductList()
    {
        InitializeComponent();

        // Translations
        this.Text = "Produkteingabe";
        this.defaultTextAddProductTextBox = "Produktname";
        this.messageMissingProductname = "Bitte einen Produktnamen angeben.";
    }

    private void textBoxAddProduct_Enter(object sender, EventArgs e)
    {
        if (this.textBoxAddProduct.Text == this.defaultTextAddProductTextBox)
        {
            this.textBoxAddProduct.ForeColor = Color.Black;
            this.textBoxAddProduct.Text = "";
        }
    }

    private void textBoxAddProduct_Leave(object sender, EventArgs e)
    {
        if (this.textBoxAddProduct.Text.Trim().Length == 0)
        {
            this.textBoxAddProduct.Text = this.defaultTextAddProductTextBox;
            this.textBoxAddProduct.ForeColor = Color.Gray;
        }
    }

    private void textBoxAddProduct_KeyUp(object sender, KeyEventArgs e)
    {
        if (e.KeyCode == Keys.Enter)
        {
            this.AddProductToList();
        }
    }

    private void buttonAddProduct_Click(object sender, EventArgs e)
    {
        this.AddProductToList();
    }

    protected bool AddProductToList()
    {
        if (this.textBoxAddProduct.Text.Trim().Length == 0)
        {
            MessageBox.Show(this.messageMissingProductname);
            return false;
        }

        decimal productPrice = Prompt.ShowPriceDialog();
        this.productBindingSource.Add(new Model.Product(this.textBoxAddProduct.Text, productPrice));
        this.textBoxAddProduct.Text = this.defaultTextAddProductTextBox;
        this.textBoxAddProduct.ForeColor = Color.Gray;
        this.dataGridViewProductList.Focus();
        return true;
    }
}
}

View/Prompt.cs

using System;
using System.Windows.Forms;
using Ferienaufgabe_2.Utility;
using System.Drawing;

namespace Ferienaufgabe_2.View
{
static class Prompt
{
    public static decimal ShowPriceDialog()
    {
        Validate validator = new Validate();

        // Create form and its fields
        Form prompt = new Form();
        Label textLabel = new Label();
        TextBox inputBox = new TextBox();
        Button confirmation = new Button();

        // Translations
        string defaultTextInputBox = "z.B.: 10,00";
        string messageInvalidPriceFormat = "Ungültige Preiseingabe! Z.B.: 10,00";
        string textOfLabel = "Preis";
        string textOfConfirmButton = "Prozess abschliessen";

        // Design form and its fields
        prompt.Width = 300;
        prompt.Height = 120;
        prompt.StartPosition = FormStartPosition.CenterParent;
        prompt.FormBorderStyle = FormBorderStyle.None;

        textLabel.Location = new Point(110, 15);
        textLabel.Text = textOfLabel;
        textLabel.Font = new Font("Arial", 18);

        inputBox.Location = new Point(40, (textLabel.Location.Y + textLabel.Height + 10));
        inputBox.Width = 205;
        inputBox.Text = defaultTextInputBox;
        inputBox.ForeColor = Color.Gray;

        confirmation.Location = new Point(40, (inputBox.Location.Y + inputBox.Height + 10));
        confirmation.Width = inputBox.Width;
        confirmation.Text = textOfConfirmButton;

        // Add events
        inputBox.KeyUp += (sender, e) => {
            if (e.KeyCode == Keys.Enter && validator.IsValidPriceFormat(inputBox.Text))
            {
                prompt.Close();
            }
            else if (e.KeyCode == Keys.Enter)
            {
                textLabel.Focus();
                MessageBox.Show(messageInvalidPriceFormat);
            }
        };

        confirmation.Click += (sender, e) => {
            if (validator.IsValidPriceFormat(inputBox.Text))
            {
                prompt.Close();
            }
            else
            {
                textLabel.Focus();
                MessageBox.Show(messageInvalidPriceFormat);
            }
        };

        inputBox.GotFocus += (sender, e) =>
        {
            if (inputBox.Text == defaultTextInputBox)
            {
                inputBox.Text = "";
                inputBox.ForeColor = Color.Black;
            }
        };
        inputBox.LostFocus += (sender, e) =>
        {
            if (inputBox.Text.Trim().Length == 0)
            {
                inputBox.Text = defaultTextInputBox;
                inputBox.ForeColor = Color.Gray;
            }
        };

        // Add fields to form
        prompt.Controls.Add(textLabel);
        prompt.Controls.Add(inputBox);
        prompt.Controls.Add(confirmation);

        // Show dialog
        prompt.ShowDialog();

        // Return converted input. This goes well anytime due to a check in the dispose actions.
        return Convert.ToDecimal(inputBox.Text);

    }
}
}

Answer 1

   if (myMatch.Success)
   {
       return true;
   }
   return false;

That's an awful lot of code for a simple thing. Why not directly return the value?

return myMatch.Success;

"myMatch" isn't a very good name either. C# is case sensitive, so you could simply use match. In fact, this whole block suffers from some poor naming.

   string strRegex = @"^[0-9]{1,}([\,]{0,1}[0-9]{1,2}){0,1}$";
   Regex myRegex = new Regex(strRegex, RegexOptions.None);
   Match myMatch = myRegex.Match(value);

The latter two can be solved by taking advantage of casing, but what about strRegex??? Why, we just call it what it is, a (regex) pattern.

    string pattern = @"^[0-9]{1,}([\,]{0,1}[0-9]{1,2}){0,1}$";
    Regex regex = new Regex(pattern, RegexOptions.None);
    Match match = regex.Match(value);

Whenever possible, avoid both static methods and "newing" objects inside your methods. Both make tests difficult to write.

public static decimal ShowPriceDialog()
{
    Validate validator = new Validate();

    // Create form and its fields
    Form prompt = new Form();
    Label textLabel = new Label();
    TextBox inputBox = new TextBox();
    Button confirmation = new Button();

Pass things as parameters, or through the ctor instead. It's called Dependency Injection, you'd do well to study it. Don't go thinking you need some special framework or anything though. Look into "Pure DI" a.k.a. "Poor man's DI".

Lastly, I think you missed the model-view-controller pattern by a bit. The controller doesn't seem to do much, while the views's code behinds are doing an awful lot of work.

Answer 2

To help you get started, here's how I would write the Product class

public class Product
{    
    public Product(string productName, decimal price)
    {
        this.ProductName = productName;
        this.Price = price;
    }

    public string ProductName { get; set; }

    public decimal Price { get; set; }    
}

I used public to denote the visibility of the class. By default, C# will mark classes as internal which may not be obvious to other people. I specifically state the visibility to avoid any confusion.

I suggest you consider using the camel case naming convention for locals, parameters, fields e.g. productName

In this case, I used auto properties e.g.

public string ProductName { get; set; }

No need to declare your own private fields (_productname), if you are not using them for anything. Consider using auto properties instead.

Answer 3

In your Model you have things a little mixed up.

private string productname;
private decimal price;

public Product(string productname, decimal price)
{
    this.Productname = productname;
    this.Price = price;
}

private string Productname
{
    set
    {
        this.productname = value;
    }
    get
    {
        return this.productname;
    }
}

private decimal Price
{
    set
    {
        this.price = value;
    }
    get
    {
        return this.price;
    }
}

You should have private properties and public accessors

Answer 4

Comments

Comments should describe why something is done in the way it is done. Describing what is done does not add any value.

Comments like

// Create form and its fields
// Translations
// Design form and its fields
.....

are also a sign that this parts maybe should be extracted to separate methods.

You can find a very good read about comments here: https://codereview.stackexchange.com/a/90113/29371

Naming

Classes should be named using nouns or noun phrases (naming guideline), so a name like Validate is better suited for a method. You should rename the class to Validator or better ProductPriceValidator to make it more clear what the class is about.
In addition the name Prompt does not reflect what the class is doing. A better name would be ProductPriceView.

ShowPriceDialog

This method is in its current state clearly too long. Also, why did you make the class and all of its methods static ?

I would make the class and its methods non static and would add methods like

• InitializeComponent() for creating the view
• ResetView() for setting the default values of the form
• and keep the ShowPriceDialog() method, but I would change the returned value from decimal to DialogResult and adding a Prize property to get the entered prize. This needs a change in the textbox eventhandlers to call Hide() on the view rather than Close().

---

Instead of using

if (this.textBoxAddProduct.Text.Trim().Length == 0)  

you should use string.IsNullOrWhiteSpace() which is checking wether the passed in string is null or does only contain white space characters.

---

Assigning string.Empty makes the intention more clear than assigning "" which by the way with older eyes not always is easy to distinguish at the first glance from " ".

---

Because you aren't changing these fields

private string defaultTextAddProductTextBox;
private string messageMissingProductname;

you should make them readonly.

Validation

IMHO the IsValidPriceFormat() has a decent flaw. You are assuming that the entered price will have a comma as decimal separator. If you plan to localize it to make it usable for e.g an american user, you would not only need to change the regex, but to provide a different validator so you won't break the german users.

A much better way to validate the passed price would be to use decimal.TryParse() which returns a boolean indicating if the given string could be parsed to a deciaml value. At the provided link location you can find a example of how to use this.

A useful component could also be a MaskedTextBox