Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub
replaced http://ux.stackexchange.com/ with https://ux.stackexchange.com/
Source Link

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
 MonthSelect = 1 ' 0001
 YearSelect = 2 ' 0010
 FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

###Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
 Dim Validator As ValidatorFlags
 If MonthSelectBox.Text = "" Then
 Validator |= ValidatorFlags.MonthSelect
 MonthErrorLabel.Text = "Please select a month."
 Else 
 MonthErrorLabel.Text = ""
 End If
 If YearSelectBox.Text = "" Then
 Validator |= ValidatorFlags.YearSelect
 YearErrorLabel.Text = "Please select a year."
 Else
 YearErrorLabel.Text = ""
 End If
 If FilePathTextBox = "" Then
 Validator |= ValidatorFlags.FilepathBox
 FilepathErrorLabel.Text = "Please select a valid filepath."
 Else
 FilepathErrorLabel.Text = ""
 End If
 Return Validator
End Sub
corrected Type of Validator, made Flags run bitwise or operation instead of add.
Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141
Loading
added flags and explanation for then
Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141
Loading
added 988 characters in body
Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141
Loading
Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141
Loading
lang-vb

AltStyle によって変換されたページ (->オリジナル) /