2
\$\begingroup\$

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
            tmpCollection.Add y
        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
        tmpFactors.Add FactorNums(FactorNums.Count)
    Else
        For x = FactorNums.Count - 1 To 1 Step -1
            If FactorNums(x) ^ 2 = NumToFactor Then
                tmpFactors.Add FactorNums(x)
                tmpFactors.Add FactorNums(x)
                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
                            tmpFactors.Add FirstFactor
                            tmpFactors.Add FactorNums(z)
                            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
\$\endgroup\$
2
\$\begingroup\$

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
        tmpFactors.Add FactorNums(FactorNums.Count)
    Else
        For x = FactorNums.Count - 1 To 1 Step -1
            If FactorNums(x) ^ 2 = NumToFactor Then
                tmpFactors.Add FactorNums(x)
                tmpFactors.Add FactorNums(x)
                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
                            tmpFactors.Add FirstFactor
                            tmpFactors.Add FactorNums(z)
                            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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Darren Bartrup-Cook May 4 '16 at 12:16
  • \$\begingroup\$ 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. \$\endgroup\$ – Darren Bartrup-Cook May 4 '16 at 12:23
  • \$\begingroup\$ The "BestFactors(7)" doesn't return 1 \$\endgroup\$ – Raystafarian May 4 '16 at 12:32
  • \$\begingroup\$ 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. \$\endgroup\$ – Darren Bartrup-Cook May 4 '16 at 13:09
0
\$\begingroup\$

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.
            dict.Add lngNumeric, lngNumeric
        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)

\$\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.