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!