# Return the best factors of a whole number

I've just finished writing my code to return the prime factors of a number and I feel the GetBestFactors function could be improved.
The GetFactors function was hacked together from the code found here:https://support.microsoft.com/en-us/kb/202782. This returns all the factors of a number in a collection.

The GetBestFactors then loops through each number finding the best set to use.
So my question is - could the GetBestFactors function use less For...Next loops?

Sub Test()

Dim tmp As Collection
Set tmp = New Collection

Dim tmp1 As Collection
Set tmp1 = New Collection

Set tmp = GetFactors(15) 'Change number - will return all factors.
Set tmp1 = GetBestFactors(15) 'Change number - will return best factors.

Debug.Assert False

End Sub

'Returns the factors of a whole number.
Public Function GetFactors(NumToFactor As Single) As Collection

Dim Count As Integer
Dim Factor As Single
Dim y As Single
Dim tmpCollection As Collection

Set tmpCollection = New Collection

Count = 0
For y = 1 To NumToFactor
Factor = NumToFactor Mod y
If Factor = 0 Then
End If
Next y

Set GetFactors = tmpCollection

End Function

'Returns the highest factors of a number.
Public Function GetBestFactors(NumToFactor As Single) As Collection

Dim tmpFactors As Collection
Dim FactorNums As Collection
Dim x As Single, y As Single, z As Single
Dim FirstFactor As Single

Set tmpFactors = New Collection
Set FactorNums = New Collection

'Get all factors for the number.
Set FactorNums = GetFactors(NumToFactor)

'If the collection has 1 item then the NumToFactor is 1.
'If there's 2 items then it's a prime number (1 and NumToFactor)
If FactorNums.Count = 1 Or FactorNums.Count = 2 Then
Else
For x = FactorNums.Count - 1 To 1 Step -1
If FactorNums(x) ^ 2 = NumToFactor Then
Exit For
Else
For y = x To 1 Step -1
FirstFactor = FactorNums(y)
For z = y - 1 To 1 Step -1
If FirstFactor * FactorNums(z) = NumToFactor Then
Exit For
End If
Next z
If tmpFactors.Count = 2 Then Exit For
Next y
End If
If tmpFactors.Count = 2 Then Exit For
Next x
End If

Set GetBestFactors = tmpFactors

End Function


I don't understand the logic specifically

allFactors(15) = 1,3,5,15
bestFactors(15) = 3,5

allFactors(7) = 1,7
bestFactors(7) = 7

allFactors(1) = 1
bestFactors(1) = 1

allFactors(0) = empty
bestFactors(0) = empty

allFactors(-15) = empty
bestFactors(-15) = empty


All of your Single variables can be Long - single is hardly used in VBA.

Your naming could use some improvement:

Sub TestForFactors()

Dim allFactors As Collection
Set allFactors = New Collection

Dim bestFactors As Collection
Set bestFactors = New Collection

Set allFactors = GetFactors(15) 'Change number - will return all factors.
Set bestFactors = GetBestFactors(15) 'Change number - will return best factors.

Debug.Assert False

End Sub


That's easier to understand.

Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names. Variable names - give your variables meaningful names. Instead of

NumToFactor = numberToFactor
Count = count
Factor = factor
tmpCollection = getFactorsCollection
tmpFactors = bestFactorsCollection
FactorNums = factorNumbers
FirstFactor = firstFactor

x,y,z = ??


All of your procedures are Public - they should be Private unless needed to be Public.

All of your function arguments are being passed ByRef - they should be passed ByVal

In terms of your logic, you're looping through every number to find factors. There are certain rules you know you can abide by - things that end with 0 or 5 - no primes end in an even number - If 5 is a factor, no need to test any multiples of 5 etc

You're using a lot of Collections - https://stackoverflow.com/questions/10579457/why-use-arrays-in-vba-when-there-are-collections you might want to try arrays or dictionaries.

In GetFactors what is Count doing? It's set to 0 then.. never used.

This is one heck of an arrow

    If FactorNums.Count = 1 Or FactorNums.Count = 2 Then
Else
For x = FactorNums.Count - 1 To 1 Step -1
If FactorNums(x) ^ 2 = NumToFactor Then
Exit For
Else
For y = x To 1 Step -1
FirstFactor = FactorNums(y)
For z = y - 1 To 1 Step -1
If FirstFactor * FactorNums(z) = NumToFactor Then
Exit For
End If
Next z
If tmpFactors.Count = 2 Then Exit For
Next y
End If
If tmpFactors.Count = 2 Then Exit For
Next x
End If


And a lot of Exit For - there's four of those against 3 For Next loops - hard to follow which exits what loop.

• Thanks for the response - I hadn't noticed the Count variable, it was left over from the code in the link. Again, the Single was left over from the code in the link - I usually use Long or Double - I think MS used Single to limit the numbers used as it's a slow process. Naming conventions was me being sloppy, sorry. :) GetFactors should probably be Private - the code was more of a stop-gap as I'd lost my way on the project I'm meant to be working on so it hasn't a specific use yet. I think naming is one of the hardest parts when coding - allfactors and bestfactors are better May 4, 2016 at 12:16
• I did consider using a Dictionary, but I've only just started getting my head around them so stuck with what I knew - I should rewrite though. The For...Next loops was the main bit I was thinking about - it didn't feel right adding all those Exit Fors but I needed a way to exit all the way out of the loops once the inner loop had populated the collection. I'll have another look at the way the logic works - makes sense with the not ending with an even number. I'm not sure what you mean by logic at the top of your post - sorry, didn't mention it's for positive numbers only. May 4, 2016 at 12:23
• The "BestFactors(7)" doesn't return 1 May 4, 2016 at 12:32
• Ah, right. 1 will only ever return 1 as the result, a prime number will only ever return 1 and itself. My line of thinking was that I'd want to figure out which numbers I need to multiply to get the originally entered number and if it's a prime number then the 1 would be superfluous to the equation so I removed it. May 4, 2016 at 13:09

Taking into account the response from Raystafarian I've rewritten the code using the Dictionary object in AllFactors (previously GetFactors) - this returns an array rather than a collection which is faster to reference.
I've also removed all the looping in BestFactors (previously GetBestFactors).
After listing all the factors for 1000 numbers it was obvious that matching factors run in order - so I just had to take the correct paring.

e.g:
The factors for 12 are: 1, 2, 3, 4, 6 & 12.
Ignoring 1 & 12 (the first set), I take 2 & 12.

The factors for 990 are: 1, 2, 3, 5, 6, 7, 10, 11, 15, 18, 22, 30, 33, 45, 55, 66, 90, 99, 110, 165, 198, 330, 495 and 990.
Ignoring 1 & 990, I take 2 and 495.

I'm not sure how much this has improved the code - but not having to loop through the factors must be a huge saving, which I'll test at some point with a really high number.

Sub Test()

'   Return all factors from 1 to 1000 in columns on Sheet1.
'
'    Dim tmp() As Variant
'    Dim x As Long, y As Long
'
'    For x = 1 To 1000
'        tmp = AllFactors(x)
'        For y = 0 To UBound(tmp)
'            Sheet1.Cells(y + 1, x) = tmp(y)
'        Next y
'    Next x

Dim tmp1 As Variant, tmp2 As Variant
Dim tmp3 As Variant
tmp1 = BestFactors(12) 'Returns 2 & 6.
tmp2 = BestFactors(990) 'Returns 2 & 495.
tmp3 = BestFactors(9) 'Returns 3 & 3.

Debug.Assert False

End Sub

'----------------------------------------------------------------------------------
' Purpose   : Returns the factors of a whole positive number.
'-----------------------------------------------------------------------------------
Private Function AllFactors(NumToFactor As Long) As Variant

Dim lngFactor As Long
Dim lngNumeric As Long
Dim dict As Object

Set dict = CreateObject("Scripting.Dictionary")

For lngNumeric = 1 To NumToFactor
lngFactor = NumToFactor Mod lngNumeric
If lngFactor = 0 Then
'No need to check if it exists -
'each value only appears once.
End If
Next lngNumeric

AllFactors = dict.items()

End Function

Public Function BestFactors(NumToFactor As Long) As Variant

Dim vAllFactors() As Variant

vAllFactors = AllFactors(NumToFactor)

Select Case UBound(vAllFactors)
Case 0, 1
'Prime number or 1.
BestFactors = vAllFactors
Case Is >= 2
BestFactors = Array(vAllFactors(UBound(vAllFactors) - 1), _
vAllFactors(LBound(vAllFactors) + 1))
End Select

End Function


Edit: I've changed the last line of code as realised it was silly - vAllFactors(UBound(vAllFactors) - (UBound(vAllFactors) - 1)) to get the second lowest value... use vAllFactors(LBound(vAllFactors) + 1)