Skip to main content
added 297 characters in body
Source Link
JK01
  • 241
  • 1
  • 10

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 :)

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.

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 :)

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 :)

Source Link
JK01
  • 241
  • 1
  • 10

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.

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 :)