patternModerate
Proper way to validate from sub to sub
Viewed 0 times
validatesubwayproperfrom
Problem
I am using a public validating sub to help validate the other subs. Is this the most efficient way to do this?
Public Sub Validator(sender As Object, e As EventArgs) Handles GrossRevenueButton.Click, CPTCodesButton.Click, EstablishedVisitsButton.Click
Dim validater As Integer = 0 'this is the variable that gets sent out from this sub
If MonthSelectBox.Text = "" Then
Label1.Text = "You didn't select a month!"
validater = 1
Else
Label1.Text = ""
End If
If YearSelectBox.Text = "" Then
Label2.Text = "You didn't select a year!"
validater = 2
Else
Label2.Text = ""
End If
If FilepathTextBox.Text = "" Then
Label3.Text = "You didn't select a file!"
validater = 3
Else
Label3.Text = ""
End If
If MonthSelectBox.Text.Length > 1 And YearSelectBox.Text.Length > 1 And FilepathTextBox.Text.Length > 1 Then
validater = 0
End If
End Sub
'GROSS REVENUE
Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles GrossRevenueButton.Click
Dim validater As Integer 'validater comes from the validate sub
If validater > 0 Then
Call Gross_Revenue()
Else
Label4.Text = "Something went wrong!"
End If
End Sub
'CPT CODES
Sub Button3_Click(sender As System.Object, e As System.EventArgs) Handles CPTCodesButton.Click
Dim validater As Integer 'validater comes from the validate sub
If validater > 0 Then
Call CPT_Totals()
Else
Label4.Text = "Something went wrong!"
End If
End Sub
'ESTABLISHED VISTS
Sub Button4_Click(sender As System.Object, e As System.EventArgs) Handles EstablishedVisitsButton.Click
Dim validater As Integer 'validater comes from the validate sub
If validater > 0 Then
Call Established_Visits()
Else
Label4.Text = "Something went wrong!"
End If
End SubSolution
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:
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
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:
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 EnumAny 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 SubCode Snippets
[Flags]
Enum ValidatorFlags As Integer
MonthSelect = 1 ' 0001
YearSelect = 2 ' 0010
FilepathBox = 4 ' 0100
End EnumPublic 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 SubContext
StackExchange Code Review Q#65191, answer score: 11
Revisions (0)
No revisions yet.