Option Explicit On
From what's an option strict and explicit?
Option Strict "restricts implicit data type conversions to only widening conversions". See here. With this option enabled, you can't accidentally convert one data type to another that is less precise (e.g. from an Integer to a Byte). Again, an option that should be turned on by default.
So after turning Option Strict On
you will get 2 errors at these lines
lblfirst.Text = Rand.Next(0, 10) 'first random number
lblsecond.Text = Rand.Next(0, 10) 'second random number
stating "Option Strict On prohibits implicit conversion of Integer to String" (my translation from german).
Now you might think that's a bad thing, but it isn't. It will just show you where you need some work to do.
And for this 2 errors it is also quite easy, you need only to call the ToString()
method of the Integer which is returned by calling the Rand.Next()
method and there is the next problem.
Variables should use camelCase for their names and also the names should be descriptive so you won't need any comments for these variables anymore, so let us do it right and rename op
to operandType
and also Rand
to randomizer
Dim randomizer As New System.Random
Dim operandType as Integer = 0
operandType = randomizer.Next(0, 3)
lblfirst.Text = randomizer.Next(0, 10).ToString() 'first random number
lblsecond.Text = randomizer.Next(0, 10).ToString() 'second random number
But wait, you are creating Integers convert them to Strings which you assign to the Text
property of the labels which you are converting to Integers again inside the check_Click()
method. That can be done better. Let declare some variables on class level to do this.
Private firstNumber As Integer = 0
Private secondNumber As Integer = 0
and change the lines above to
firstNumber = randomizer.Next(0, 10)
lblfirst.Text = firstNumber.ToString() 'first random number
secondNumber = randomizer.Next(0, 10)
lblsecond.Text = secondNumber.ToString() 'second random number
now we can use firstNumber
and secondNumber
in the check_Click
method without using the String to Integer conversation (I will later talk about this also).
But wait, we can do better... Why don't we calculate the desired result at the time we generate the captcha ? Let us declare another Integer variable to hold the result
Private internalResult As Integer = 0
and use it
If operandType = 0 Then ' Operator selection
internalResult = firstNumber + secondNumber
ElseIf operandType = 1 Then
internalResult = firstNumber - secondNumber
ElseIf operandType = 2 Then
internalResult = firstNumber * secondNumber
End If
So looking at this If..ElseIf..ElseIf
I see 2 things, operandType is in the Range[0..2] which should be expressed as an enum (which I will lend from ckuhn203's answer) and the If..ElseIf..
should be a Select Case
. We need also to change
Private operandType As Integer
to
Private operandType As Operand
next we add the enum
Enum Operand
Add
Substract
Multiply
End Enum
and implement the Select..Case
Select Case operandType
Case Operand.Add
internalResult = firstNumber + secondNumber
Case Operand.Substract
internalResult = firstNumber - secondNumber
Case Operand.Multiply
internalResult = firstNumber * secondNumber
End Select
Now let us talk about conversion from String
to Integer
by taking a look at the last few lines of your check_Click()
method
If output = CInt(txtResult.Text) Then
MsgBox("Successfully varified")
Else
MsgBox("Varification Faild")
End If
You are calling CInt(txtResult.Text)
. Assume the user will enter "lala" in the textbox. This will result in an InvalidCastException
. A better way would be to use Integer.TryParse() which as the name implies tries to parse a String to an Integer. If this succeeds the method returns true
otherwise it will return false
. So let us use it
If (Integer.TryParse(txtResult.Text, output) AndAlso output = internalResult) Then
MsgBox("Successfully verified")
Else
MsgBox("Verification failed")
End If
Wait, what is this AndAlso
? AndAlso performs a short-circuiting logical conjunction on two expressions. So this means, that the right part of the AndAlso
will only be evaluated, if the left part is true
.
As we have, putting aside the last part, only code which belongs to the captcha generating and checking, so it would be a good thing to create a class of it
Public Class Captcha
Private Enum Operand
Add
Substract
Multiply
End Enum
Private firstNumber As Integer = 0
Private secondNumber As Integer = 0
Private internalResult As Integer = 0
Private operandType As Operand
Private result As Integer = 0
Private randomizer As New Random()
Public Function IsValid(answer As String) As Boolean
internalResult = CalculateResult()
Return (Integer.TryParse(answer, result) AndAlso result = internalResult)
End Function
Private Function CalculateResult() As Integer
Dim result As Integer = 0
Select Case operandType
Case Operand.Add
result = firstNumber + secondNumber
Case Operand.Substract
result = firstNumber - secondNumber
Case Operand.Multiply
result = firstNumber * secondNumber
End Select
Return result
End Function
Public Function Generate() As String
Dim generatedCaptcha As String = String.Empty
operandType = CType(randomizer.Next(0, 3), Operand)
Select Case operandType
Case Operand.Add
generatedCaptcha = " + "
Case Operand.Substract
generatedCaptcha = " - "
Case Operand.Multiply
generatedCaptcha = " * "
End Select
firstNumber = randomizer.Next(0, 10)
secondNumber = randomizer.Next(0, 10)
Return firstNumber.ToString() & generatedCaptcha & secondNumber.ToString()
End Function
End Class
Now you need only one label to show the captcha which I have named captchaLabel
and your code looks fine and tidy
Dim _captcha As New Captcha()
Private Sub Generate_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Generate.Click
captchaLabel.Text = _captcha.Generate()
End Sub
Private Sub check_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles check.Click
If _captcha.IsValid(txtResult.Text) Then
MessageBox.Show("Successfully verified")
Else
MessageBox.Show("Verification Failed")
End If
End Sub
Side Note
As I see that you are comming from VB6, I want to encourage you to learn the .NET way of these things. So skip using MsgBox(),CInt(), Mid() etc, as these are only kept to stay backward compatible.
Option Strict On
which you should use as default, this code does not compile. – Heslacher Aug 4 '14 at 14:24Compare the user input and expression evaluation result for confirmation.
where can I find this in your code ? . – Heslacher Aug 5 '14 at 9:09