Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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: http://codereview.stackexchange.com/a/90113/29371https://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

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: http://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

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

deleted 224 characters in body
Source Link
Heslacher
  • 50.4k
  • 5
  • 81
  • 175

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: http://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.


Why do you validate the input in the KeyUp event of the textbox ? This should be done in the Click event of the confirmation button only. A user of your application would ask himself why did the form close ?

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

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: http://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.


Why do you validate the input in the KeyUp event of the textbox ? This should be done in the Click event of the confirmation button only. A user of your application would ask himself why did the form close ?

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

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: http://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

Source Link
Heslacher
  • 50.4k
  • 5
  • 81
  • 175

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: http://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.


Why do you validate the input in the KeyUp event of the textbox ? This should be done in the Click event of the confirmation button only. A user of your application would ask himself why did the form close ?

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