5
\$\begingroup\$

Nowadays many sites introduce CAPTCHAs (Completely Automated Public Turing test to tell Computers and Humans Apart) for securing their sites. I wish to introduce the technique in windows forms, as expression evaluation CAPTCHA.

Approach:

  • Two randomly generated integers are loaded into two different labels
  • Place a random operator (+,-,*) in between them
  • Users are requested to provide the calculated result in a text box
  • Compare the user input and expression evaluation result for confirmation.

Private Sub Generate_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Generate.Click
    ' This function will generate the Numeric Captcha 
    ' with two random number and one operator
    Dim Rand As New System.Random
    op = Rand.Next(0, 3)
    lblfirst.Text = Rand.Next(0, 10) 'first random number
    lblsecond.Text = Rand.Next(0, 10) 'second random number
    If op = 0 Then ' Operator selection
        lblop.Text = "+"
    ElseIf op = 1 Then
        lblop.Text = "-"
    ElseIf op = 2 Then
        lblop.Text = "*"
    End If
End Sub

Private Sub check_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles check.Click
    'Validating the user input with Random CAPTCHA
    Dim output As Integer
    If op = 0 Then ' choosing operator based on op
        output = CInt(lblfirst.Text) + CInt(lblsecond.Text)
    ElseIf op = 1 Then
        output = CInt(lblfirst.Text) - CInt(lblsecond.Text)
    ElseIf op = 2 Then
        output = CInt(lblfirst.Text) * CInt(lblsecond.Text)
    End If
    If output = CInt(txtResult.Text) Then
        MsgBox("Successfully varified")
    Else
        MsgBox("Varification Faild")
    End If
End Sub

Are there any suggestions on improving the technique?

\$\endgroup\$
  • \$\begingroup\$ Any specific case where this would be an advantage? if someone would like to automate it, it could simply put read the label. Other than that the code is pretty straight forward. i would try to reduce the duplicate code by creating a specific function for it. \$\endgroup\$ – Viezevingertjes Aug 4 '14 at 13:49
  • 2
    \$\begingroup\$ With Option Strict On which you should use as default, this code does not compile. \$\endgroup\$ – Heslacher Aug 4 '14 at 14:24
  • \$\begingroup\$ Why not a finger print scanner ? \$\endgroup\$ – bhathiya-perera Aug 4 '14 at 17:43
  • \$\begingroup\$ @ Bhathiya-JaDogg-Perera : it is costly and need to keep database, my proposed method is simplest \$\endgroup\$ – Sujith Karivelil Aug 5 '14 at 3:25
  • \$\begingroup\$ Compare the user input and expression evaluation result for confirmation. where can I find this in your code ? . \$\endgroup\$ – Heslacher Aug 5 '14 at 9:09
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This is a really good review. I thought about mentioning a captcha class, but figured I was already throwing a lot at OP. In hindsight, I should have. \$\endgroup\$ – RubberDuck Aug 5 '14 at 15:48
4
\$\begingroup\$
  • You should use Option Strict and Option Explicit. They magically turns runtime errors into compiler errors.
  • I'm not a fan of seeing a bunch of logic that isn't related to the GUI in GUI event procedures. Separate the logic into separate subroutines and call them from the click events. Eventually you'll want to call this logic from somewhere else. Let's remove the temptation to raise events instead of calling procedures now. The first event procedure should look like this:

    Private Sub Generate_Click()
        GenerateCaptcha
    End Sub
    
  • Variables should be camelCased. Dim Rand should be Dim rand.

  • If you need a comment to explain what a variable is, you should rename that variable.

    If op = 0 Then ' Operator selection
    
  • Same line, what you want here is a Select Case. More about the Select statement.

  • Define an enumeration for your operationSelector values. It will clean up your Selects.

     Private Enum Operation
         add = 0
         subtract
         multiply
     End Enum
    
  • In general, this should be split up into smaller functions/subs. Each one should have a single responsibility.

    Private Sub Generate_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Generate.Click
         ' This function will generate the Numeric Captcha 
         ' with two random number and one operator
         Dim rand As New System.Random
         Dim operationSelector As Integer
    
         operationSelector = Rand.Next(0, 3)
         lblfirst.Text = Rand.Next(0, 10) 'first random number
         lblsecond.Text = Rand.Next(0, 10) 'second random number
    
         SetLblOp operationSelector
    
    End Sub
    
    Private Sub SetLblOp(operationSelector as Operation)
    
         Select Case operationSelector
             Case Operation.add
                 lblop.Text = "+"
             Case Operation.subtract
                 lblop.Text = "-"
             Case Operation.multiply
                 lblop.Text = "*"
         End Select
    End Sub
    
    
    Private Sub check_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles check.Click
        'Validating the user input with Random CAPTCHA
    
        If CalculateCaptcha = CInt(txtResult.Text) Then
            MsgBox("Successfully varified")
        Else
            MsgBox("Varification Faild")
        End If
    End Sub
    
     Private Function CalculateCaptcha(operationSelector As Operation) As Integer
         Select Case operationSelector
             Case Operation.add
                 Return CInt(lblfirst.Text) + CInt(lblsecond.Text)
             Case Operation.subtract
                 Return CInt(lblfirst.Text) - CInt(lblsecond.Text)
             Case Operation.multiply
                 Return CInt(lblfirst.Text) * CInt(lblsecond.Text)
         End Select
     End sub
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.