PriceDialog - Prompt & price validation

Question

In a recent CodeReview I presented a Product Manager that manages product listings and prices. Based on the great feedback I have refactored the mentioned Prompt and validator class and want feedback about the code and naming.

Utilities/ProductValidator.cs

namespace Ferienaufgabe_2.Utilities
{
class ProductValidator
{
 public bool IsValidPriceFormat(string value)
 {
 decimal result;
 return (decimal.TryParse(value, out result) && result >= 0 );
 }
}
}

View/PriceDialog.cs

using Ferienaufgabe_2.Utilities;
using System;
using System.Drawing;
using System.Windows.Forms;
namespace Ferienaufgabe_2.View
{
class PriceDialog
{
 private ProductValidator ProductValidator { get; set; }
 private Form DialogBox { get; set; }
 private Label TextLabel { get; set; }
 private TextBox InputBox { get; set; }
 private Button ButtonConfirmation { get; set; }
 private const string DefaultTextInputBox = "z.B.: 10,00";
 private const string MessageInvalidPriceFormat = "Ungültige Preiseingabe! Z.B.: 10,00";
 private const string TextOfLabel = "Preis:";
 private const string TextOfConfirmButton = "Prozess abschliessen";
 public PriceDialog()
 {
 this.InitializeComponent();
 this.DesignComponent();
 this.AddEventsToComponent();
 this.AddComponentToDialog();
 }
 private void InitializeComponent()
 {
 this.ProductValidator = new ProductValidator();
 this.DialogBox = new Form();
 this.TextLabel = new Label();
 this.InputBox = new TextBox();
 this.ButtonConfirmation = new Button();
 }
 private void DesignComponent()
 {
 this.DialogBox.Width = 300;
 this.DialogBox.Height = 120;
 this.DialogBox.StartPosition = FormStartPosition.CenterParent;
 this.DialogBox.FormBorderStyle = FormBorderStyle.None;
 this.TextLabel.Location = new Point(110, 15);
 this.TextLabel.Text = TextOfLabel;
 this.TextLabel.Font = new Font("Arial", 18);
 this.InputBox.Location = new Point(40, (this.TextLabel.Location.Y + this.TextLabel.Height + 10));
 this.InputBox.Width = 205;
 this.InputBox.Text = DefaultTextInputBox;
 this.InputBox.ForeColor = Color.Gray;
 this.ButtonConfirmation.Location = new Point(40, (this.InputBox.Location.Y + this.InputBox.Height + 10));
 this.ButtonConfirmation.Width = InputBox.Width;
 this.ButtonConfirmation.Text = TextOfConfirmButton;
 }
 private void AddEventsToComponent()
 {
 this.InputBox.KeyUp += (sender, e) => {
 if (e.KeyCode == Keys.Enter && this.ProductValidator.IsValidPriceFormat(this.InputBox.Text))
 {
 this.DialogBox.Close();
 }
 else if (e.KeyCode == Keys.Enter)
 {
 this.TextLabel.Focus(); // Otherwise MessageBox cannot be canceled via {enter}
 MessageBox.Show(MessageInvalidPriceFormat);
 }
 };
 this.ButtonConfirmation.Click += (sender, e) => {
 if (this.ProductValidator.IsValidPriceFormat(InputBox.Text))
 {
 this.DialogBox.Close();
 }
 else
 {
 this.TextLabel.Focus(); // Otherwise MessageBox cannot be canceled via {enter}
 MessageBox.Show(MessageInvalidPriceFormat);
 }
 };
 this.InputBox.GotFocus += (sender, e) =>
 {
 if (this.InputBox.Text == DefaultTextInputBox)
 {
 this.InputBox.Text = "";
 this.InputBox.ForeColor = Color.Black;
 }
 };
 this.InputBox.LostFocus += (sender, e) =>
 {
 if (this.InputBox.Text.Trim().Length == 0)
 {
 this.InputBox.Text = DefaultTextInputBox;
 this.InputBox.ForeColor = Color.Gray;
 }
 };
 }
 private void AddComponentToDialog()
 {
 this.DialogBox.Controls.Add(this.TextLabel);
 this.DialogBox.Controls.Add(this.InputBox);
 this.DialogBox.Controls.Add(this.ButtonConfirmation);
 }
 public decimal Show()
 {
 this.DialogBox.ShowDialog();
 // This goes well anytime due to a check in the dispose actions.
 return Convert.ToDecimal(this.InputBox.Text);
 }
}
}

• JK01: You don't have any modifier in front of any of your class declarations, which by default makes them internal. If you meant to make them internal, that's fine, but at least write internal class { ... }, so its clear that you didn't do it by accident. If it is an accident then decide on a modifier and add it.
• RubberDuck: @JK01 that sounds like a valid answer to me.
• Alexander: It's an accident. Going to choose one tomorrow. Thanks
• JK01: I wonder, what is the relationship between the user @RubberDuck and the tag [rubberduck] and the rubberduck (codereview.stackexchange.com/q/96319/5701) question? :)
• BCdotWEB: @JK01 Probably all inspired by this.

Answer 1

class PriceDialog
{
 private ProductValidator ProductValidator { get; set; }
 private Form DialogBox { get; set; }
 private Label TextLabel { get; set; }
 private TextBox InputBox { get; set; }
 private Button ButtonConfirmation { get; set; }

Not quite how MVC works. Your view should implement an interface. That interface won't expose anything from the UI. It should only expose the data you need to work with, not UI controls.

class PriceDialog : IPriceView
{
 string SomeText 
 {
 get { return TextBox.Text; }
 set { TextBox.Text = value; }
 }
 //...
}

Then a presenter or controller, depending on the flavor of MVC, interacts with the view and responds to its events. This makes your code testable, but there are other benefits. It allows you to easily swap out one view for another without changing the controller. As it is right now, you'd have to duplicate a lot of this logic in order to implement a console interface. By making your view anemic and interacting through an interface, you decouple the concept of a PriceView from this particular implementation of it.

Answer 2

Modifiers

You don't have any modifier in front of any of your class declarations, which by default makes them internal.

If you meant to make them internal, that's fine, but at least write internal class { ... }, so its clear that you didn't do it by accident.

If it is an accident then decide on a modifier and add it. public is usually a good choice.

If you are implementing an interface, make the interface public and the implementation internal or protected. Then it is only accessible by using the interface.

Naming

public bool IsValidPriceFormat(string value) does not actually check if value is a valid price format. Format implies that it is a string used to format the display of something. Your function actually checks if the value is a valid decimal>= 0. A name of IsValidPrice would be more correct.

If you were actually intending to check that the price is in a valid format (MessageInvalidPriceFormat string does say to the user that it has to have a comma eg "10,00"), then your method is not implemented correctly, since a value of "10" or "10.00" would also pass the is valid test.

ProductValidator only validates the price, so it should really be named ProductPriceValidator

Handling validation

The way you handle validation is a bit unusual with the repeated check for e.KeyCode == Keys.Enter and the repeated handling of the validation. Try refactoring the duplicate code out.

private void HandleValidation()
{
 if (this.ProductValidator.IsValidPriceFormat(InputBox.Text))
 {
 this.DialogBox.Close();
 }
 else
 {
 this.TextLabel.Focus();
 MessageBox.Show(MessageInvalidPriceFormat);
 }
}
this.InputBox.KeyUp += (sender, e) => {
 if (e.KeyCode == Keys.Enter)
 {
 HandleValidation();
 } 
};
this.ButtonConfirmation.Click += (sender, e) => {
 HandleValidation();
};

this

I don't think you need to use the this keyword in front of everything.

Namespace

namespace Ferienaufgabe_2.Utilities - Utilities is not a good name space, nor is Helpers or Managers or anything else vague. These are namespaces that scream "hey, put any old class in here, I don't care!"

namespace Ferienaufgabe_2.Validators would be much better.

Dependency Injection

Instead of creating a new ProductValidator() you can have it automatically created for you by dependency injection. But that's a subject for another day :)

• Alexander: You cannot know that but the class ProductValidator will have more methods later when Product has more attributes.
• Alexander: I've noticed "10.00" becomes "1000". I am in Germany and using Convert.ToDecimal(this.InputBox.Text);. Is there a way to allow both comma and dot or do I have to replace the dot by comma? In Germany some people use dot instead of comma because they know that some software expects a dot. As of this I want to allow both.