views:

403

answers:

3

Hi

I'm analyzing a code from the website and I tried it on my side as well but seems it doesn't work. Could you please tell me why? would greatly appreciate your help.

Thanks

Private Sub CommandButton1_Click()
    Dim N, D As Single
    Dim tag As String

    N = Cells(2, 2)
    Select Case N
     Case Is < 2
      MsgBox "It is not a prime number"
     Case Is = 2
      MsgBox "It is a prime number"
     Case Is > 2
      D = 2
      Do
       If N / D = Int(N / D) Then
        MsgBox "It is not a prime number"
        tag = "Not Prime"
        Exit Do
       End If
       D = D + 1
      Loop While D <= N - 1
      If tag <> "Not Prime" Then
       MsgBox "It is a prime number"
      End If
    End Select
End Sub
+6  A: 

The single biggest problem I see is using Single instead of Integer or Long. Primes are positive integers and are not thought of in the context of decimal values (as far as I know). Thus by using a singles and comparing them to divided ints, you're opening yourself up to nasty edge-case rouding errors.

The line If N / D = Int(N / D) Then is using a poor method to see whether or not the numbers are prime. It's assuming that every time you divide a floating point number (in this case, the single) by the divisor, if it has a decimal remainder, then the integer conversion of that remainder will not be equal. However, I've run into rounding errors sometimes with floating point numbers when trying to compare answers, and in general, I've learned to avoid using floating point to int conversions as a way of comparing numbers.

Here's some code you might try instead. Some things to note:

  • I've changed the types of N and D so that they are Longs and not Singles. This means they are not floating point and subject to possible rounding errors.
  • I've also explicitly converted the cell value to a long. This way you know in your code that you are not working with a floating point type.
  • For the comparison, I've used Mod, which returns the remainder of the N divided by D. If the remainder is 0, it returns true and we know we don't have a prime. (Note: Remainder is often used with \, which only returns the integer value of the result of the division. Mod and \ are commonly used in precise division of integer types, which in this case is very appropriate.
  • Lastly, I changed your message box to show the actual number being compared. Since the number in the cell is converted, if the user enters a floating point value, it will be good for them to see what it was converted to.

You'll probably also note that this code runs a lot faster than your code when you get to high numbers in the hundreds of millions. '

Sub GetPrime()
Dim N As Long
Dim D As Long
Dim tag As String

N = CLng(Cells(2, 2))

Select Case N
    Case Is < 2
        MsgBox N & " is not a prime number"
    Case Is = 2
        MsgBox N & " is a prime number"
    Case Is > 2
        D = 2
        Do
            If N Mod D = 0 Then
                MsgBox N & " is not a prime number"
                tag = "Not Prime"
                Exit Do
            End If
            D = D + 1
        Loop While D <= N - 1
        If tag <> "Not Prime" Then
            MsgBox N & " is a prime number"
        End If
End Select
End Sub


NOTE: I changed the name of the procedure to be GetPrime. In your code, you had:

Private Sub CommandButton1_Click()

In the line above, you are defining a procedure (also called a method or sometimes just referred to as a sub). The word Sub indicates you are defining a procedure in code that returns no value. (Sometimes you might see the word Function instead of Sub. This means the procedure returns a value, such as Private Function ReturnANumber() As Long.) A procedure (Sub) is a body of code that will execute when called. Also worth noting, an excel macro is stored in VBA as a Sub procedure.

In your line of code, CommandButton1_Click() is the name of the procedure. Most likely, this was created automatically by adding a button to an Excel spreadsheet. If the button is tied to the Excel spreadsheet, CommandButton1_Click() will execute each time the button is pressed.

In your code, Private indicates the scope of the procedure. Private generally means that the procedure cannot be called outside of the module or class in which it resides. In my code, I left out Private because you may want to call GetPrime from a different module of code.

You mentioned in your comments that you had to change the name of my procedure from GetPrime() to CommandButton1_Click(). That certainly works. However, you could also have simply called GetPrime from within CommandButton1_Click(), like below:

Private Sub CommandButton1_Click()
    'The following line of code will execute GetPrime()  '
    'Since GetPrime does not have parameters and does not return a value, '
    'all you need to do is put the name of the procedure without the ()   '
    GetPrime
End Sub

'Below is the entire code for the Sub GetPrime()    '
Sub GetPrime()
    'The body of the code goes below: '
    ' ... '

End Sub

Hopefully this helped to explain a little bit about VBA to further your understanding!

Ben McCormack
@bmccormack:thanks for explanation.let me read it again as I'm off to train now.I'll check it again once I get home
tintincute
Also worth noting, I tested a prime greater than 100,000,000 and my code returned a result in about 15 seconds. The code you provided never even finished after minutes of running.
Ben McCormack
@bmccormack: i also tried your example and this doesn't work if I didn't change the "Sub GetPrime()" into "Private Sub CommandButton1_Click()" i'm just starting to learn VBA i don't understand this Sub GetPrime beginning.What's the difference?if you do your test do you write it in VBA editor like the one you posted? thanks
tintincute
@tintincute Very good jub in making your code work by changing the name of the procedure! I'll edit the bottom of my post to explain a little bit about how procedures work.
Ben McCormack
+2  A: 

I'm not sure where you copied this code from, but it's terribly inefficient. If I may:

  1. Dim N, D As Long will cause D to be a Long, and N to be a variant. As you may know, variants are one of the slowest data types available. This line should be: Dim N As Long, D As Long
  2. You only need to test every other number as an even number will always be divisible by two. (Therefore can not possibly be prime).
  3. You don't need to test all the way up to N. You only need to test up to the Square Root of N. This is because after the square root the factors just switch sides, so you are just retesting values.
  4. For Loops only evaluate the For-Line once for the life of the loop, but Do and While loops evaluate their conditional on every loop, so N-1 is being evaluated many, many times. Store this value in a variable if you want to use a Do Loop.

Ok, so now that we have dispensed with the blah, blah, blah, here is the code. I structured it so you can use it as a UDF from Excel as well (Ex: =ISPRIME(A2)):

Option Explicit

Sub GetPrime()
    Dim varValue As Variant
    varValue = Excel.ActiveSheet.Cells(2&, 2&).Value
    If IsNumeric(varValue) Then
        If CLng(varValue) = varValue Then
            If IsPrime(varValue) Then
                MsgBox varValue & " is prime", vbInformation, "Prime Test"
            Else
                MsgBox varValue & " is not prime", vbExclamation, "Prime Test"
            End If
            Exit Sub
        End If
    End If
    MsgBox "This operation may only be performed on an integer value.", vbCritical, "Tip"
End Sub

Public Function IsPrime(ByVal num As Long) As Boolean
    Dim lngNumDiv As Long
    Dim lngNumSqr As Long
    Dim blnRtnVal As Boolean
    ''//If structure is to optimize logical evaluation as AND/OR operators do not
    ''//use short-circuit evaluation in VB.'
    If num = 2& Then
        blnRtnVal = True
    ElseIf num < 2& Then 'Do nothing, false by default.
    ElseIf num Mod 2& = 0& Then 'Do nothing, false by default.
    Else
        lngNumSqr = Sqr(num)
        For lngNumDiv = 3& To lngNumSqr Step 2&
            If num Mod lngNumDiv = 0& Then Exit For
        Next
        blnRtnVal = lngNumDiv > lngNumSqr
    End If
    IsPrime = blnRtnVal
End Function
Oorang
@Oorang: I copied it from here: http://www.vbtutor.net/VBA/vba_tutorial.html
tintincute
+2  A: 

You can optimise it further (and make it more readable, in my opinion) by making the following changes. First performance:

  • Use longs, not floats. This will result in a huge speed increase.
  • You don't need to check up to n-1, only the square root of n. That's because if a factor d greater than sqrt(n) exists, its counterpart n/d would have already been found under sqrt(n). We use a special variable for this so that we don't get overflow by calculating divisor2. It also speeds it up by calculating that once rather than calculating the square every time through the loop (even though getting the square root is undoubtedly slower than squaring, it only happens once).
  • Do a special check first for multiples of two then you need only check that your number is a multiple of an odd number, effectively doubling the speed (not checking if you're a factor of a multiple of two).
  • Use the modulo operator rather than division/multiplication.

Now readability:

  • Use descriptive variable names.
  • Use a boolean for boolean values (not a string like tag).
  • Move the message box logic down to the bottom, based on the isPrime boolean, rather than scattering the messages amongst your code.

With all those changes, the following code can detect a 9-digit prime number (795,028,841) in well under a second. In fact, we can detect the largest 31-bit prime (2,147,483,647) in the same time.

Based on benchmarks (putting a 10,000-iteration for loop around the select), it takes 35 seconds on my box to detect that 31-bit prime. That's about 285 times per second - hopefully that'll be fast enough for you :-)

Option Explicit

Public Sub Go()
    Dim number As Long
    Dim divisor As Long
    Dim maxdivisor As Long
    Dim isPrime As Boolean

    number = CLng(Cells(2, 2))
    Select Case number
        Case Is < 2
            isPrime = False
        Case Is = 2
            isPrime = True
        Case Is > 2
            isPrime = True
            If number mod 2 = 0 Then
                isPrime = False
            Else
                maxdivisor = CLng(Sqr(number)) + 1
                divisor = 3
                Do
                    If number mod divisor = 0 Then
                        isPrime = False
                        Exit Do
                    End If
                    divisor = divisor + 2
                Loop While divisor <= maxdivisor
            End If
    End Select
    If isPrime Then
        MsgBox "Number (" & number & ") is prime"
    Else
        MsgBox "Number (" & number & ") is not prime"
    End If
End Sub
paxdiablo
@paxdiablo: the line which says: "If number rem divisor = 0 Then" is being marked as red. Could you please check at your side? Do you also have the same problem? The code will not run.
tintincute
Sorry, that should be 'mod' as in 'modulo', not 'rem' as in 'remainder' or, as VBA rightly sees is, 'rem' as in 'remark' :-)
paxdiablo
no problem I changed that but, it doesn't work. No display messagebox to say if its prime or not prime number. Nothing happened:-(
tintincute
Works fine here :-) I'd suggest putting a msgbox at the start to make sure it's being called. In addition, are you getting the hourglass icon when you run it?
paxdiablo
no I'm not getting anything. I just put a box where I can click it to show the message box after but it doesn't happen.hmm...
tintincute
i see the problem now. In your code you use: "Public Sub Go()" and I changed it into: "Private Sub CommandButton1_Click()" then it works now. Would you be so kind to explain what is "Public Sub Go()" means? thanks
tintincute
I did that just out of habit since I tend to run macros by assigning them to CTRL-SHIFT-A and such - from memory, you have to have it public to chow up in the macro dialog box as being able to have a key assigned to it (and Go is just a random name). Sorry about that.
paxdiablo
no problem thanks for explaining and now it's working:-)
tintincute