Excel VBA Recursive function return not the expected result

Question

I have the following function Which Is calling itself (Recursive). The goal is to return a unique filename formatted as filename (1).ext, filename (2).ext, etc.

Function CreateUniqueFileName(strPath As String, strFileName, orderId As Integer) As String
Dim extPos As Integer
Dim extension As String
Dim fileName As String

fileName = ""

extPos = InStrRev(strFileName, ".")

If (extPos > 0) Then
    fileName = Left(strFileName, extPos - 1)
    extension = Right(strFileName, Len(strFileName) - extPos)

    If (orderId = 0) Then
        fileName = strFileName
        CreateUniqueFileName = fileName
    Else
        fileName = fileName & " (" & CStr(orderId) & ")." & extension
    End If

    If (DoesFileExist(strPath & fileName)) Then
        Call CreateUniqueFileName(strPath, fileName, orderId + 1)
    Else
        CreateUniqueFileName = fileName
        Exit Function
    End If
End If
End Function

If it is called the first time and the orderId value is 0 this is always the first one and therefore unique. So in that case the function is only called once. But when the recursion is executed and the DoesFileExists returns false the return value should return the generated filename and exit. However, when I debug the function is executing without errors but it always returns the original value instead of the result of the original iteration.

So for example, if I call this function like this: CreateUniqueFileName("C:\Temp\",""1010-40-800.jpg",1) It checks in C:\temp if there is already a file named 1010-40-800 (1).jpg, if so the same function is called and the orderId is updated by 1 in this case CreateUniqueFileName("C:\Temp\",""1010-40-800.jpg",2). The same process is repeated (Recusive). Now assume the 1010-40-800 (2).jpg is unique (File is not found). I would expect the function to return 1010-40-800 (2).jpg as as string result. But instead it will return the value 1010-40-800 (1).jpg. Which is actually the value of the first time the function is called.

What am I missing here?

@braX, I do use the Dir function to check if a file exists, that assumption is correct. So you say I have to use the FSO for checking if a file exists would solve this problem? — Stephan
– Stephan, Commented Nov 3, 2019 at 8:22

Storax · Accepted Answer · 2019-11-03 09:41:35Z

1

You just have a small flaw in your code when you call your function recursively. Try this

Function CreateUniqueFileName(strPath As String, strFileName, orderId As Integer) As String
    Dim extPos As Integer
    Dim extension As String
    Dim fileName As String

    fileName = ""

    extPos = InStrRev(strFileName, ".")

    If (extPos > 0) Then
        fileName = Left(strFileName, extPos - 1)
        extension = Right(strFileName, Len(strFileName) - extPos)

        If (orderId = 0) Then
            fileName = strFileName
            CreateUniqueFileName = fileName
        Else
            fileName = fileName & " (" & CStr(orderId) & ")." & extension
        End If

        If (DoesFileExist(strPath & fileName)) Then
            CreateUniqueFileName = CreateUniqueFileName(strPath, fileName, orderId + 1)
        Else
            CreateUniqueFileName = fileName
            'Exit Function
        End If
    End If
End Function

This is still not giving you what you want as it appends every orderID but you should see the flaw and hopefully be able to fix the remaining issue.

I used the following function to check if a file exists

Function DoesFileExist(fullFileName As String) As Boolean

    Dim TestStr As String
    TestStr = ""
    On Error Resume Next
    TestStr = Dir(fullFileName)
    On Error GoTo 0
    If TestStr = "" Then
        DoesFileExist = False
    Else
        DoesFileExist = True
    End If

End Function

But in this case IMO a loop would be better for getting a unique file name.

Update: Find attached the completely fixed version for the recursive call and a "loop" version

 Function CreateUniqueFileName(strPath As String, strFileName, orderID As Integer) As String
    Dim extPos As Integer
    Dim extension As String
    Dim fileName As String
    Dim resFilename As String

    extPos = InStrRev(strFileName, ".")

    If (extPos > 0) Then
        fileName = Left(strFileName, extPos - 1)
        extension = Right(strFileName, Len(strFileName) - extPos)

        If (orderID = 0) Then
            resFilename = strFileName
        Else
            resFilename = fileName & " (" & CStr(orderID) & ")." & extension
        End If

        If (DoesFileExist(strPath & resFilename)) Then
            CreateUniqueFileName = CreateUniqueFileName(strPath, strFileName, orderID + 1)
        Else
            CreateUniqueFileName = resFilename
        End If

    End If
End Function

And the version with a loop

Function CreateUniqueFileNameA(strPath As String, strFileName) As String

    Dim extPos As Integer
    Dim extension As String
    Dim fileName As String
    Dim resFilename As String
    Dim orderID As Long

    extPos = InStrRev(strFileName, ".")

    If extPos > 0 Then

        fileName = Left(strFileName, extPos - 1)
        extension = Right(strFileName, Len(strFileName) - extPos)
        orderID = 0

        resFilename = strFileName
        Do While DoesFileExist(strPath & resFilename)
            orderID = orderID + 1
            resFilename = fileName & " (" & CStr(orderID) & ")." & extension
        Loop

    End If

    CreateUniqueFileNameA = resFilename

End Function

edited Nov 3, 2019 at 9:41

answered Nov 3, 2019 at 8:44

Storax

12.7k3 gold badges21 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Stephan Over a year ago

I overlooked that. Why would a loop be better? Because of performance or stack issues when using recursion? Anyway, both of these solutions will solve my problem. My DoesFileExists looks the same as yours. So this should work well.

Storax Over a year ago

Probably a matter of taste but a loop is clearer in this case, at least for me.

DisplayName Over a year ago

@Storax, as a side note, in DoesFileExist() you can change the If TestStr = "" Then... Else" to simply DoesFileExist = Not (TestStr = "") . And you'd better use vbNullString in lieu of all those ""

Storax Over a year ago

I just copied the function from here but you are certainly right one could improve this function. I just wanted to clarify that you can use Dir in a recursive call like that.

Stephan Over a year ago

@Storax to be honest my DoesFileExists() looks like: Function DoesFileExist(strFullPath As String) As Boolean If Len(Dir(strFullPath)) = 0 Then DoesFileExist = False Else DoesFileExist = True End If End Function I use the LEN function to test. I like the Not-part however.

|

freeflow · Accepted Answer · 2019-11-03 10:33:44Z

There are structural, logical and assumption issues with your code.

The structure issue is that the code for splitting off the extension encompasses your recursion call so your recursion will never occur if the filename does not contain an extension. If this is a deliberate decision then its better to exit the function early rather than encompass everything else in an if end if.

Your logical error is that you don't use the recursive call of the function correctly

Call CreateUniqueFileName(strPath, fileName, orderId + 1)

Should be

CreateUniqueFileName = CreateUniqueFileName(strPath, fileName, orderId + 1)

Your assumption issue is that the arguments to your function are values. They are not. By default VBA passes parameters by reference so in your code 'filename' is the same variable every time the function is called rather than being a new copy.

Hence this line

fileName = fileName & " (" & CStr(orderId) & ")." & extension

will just cause filename problems as you do the recursion with filename rather than strFilename.

I've restructured your code to make the recursive part cleaner (although as observed by others a loop would be much preferred)

Function CreateUniqueFileName(ByVal StrPath As String, ByVal strFileName, ByRef orderId As Integer) As String

Dim FileNameArray                                As Variant

    FileNameArray = Split(strFileName, ".")

    If Len(FileNameArray(1)) = 0 Then

        Debug.Print ("CreateUniqueFilename says strFilename has no extension")
        CreateUniqueFileName = vbNullString
        Exit Function

    End If

    If orderId = 0 Then

       CreateUniqueFileName = FileNameArray(0) & Format(orderId, "0000") & FileNameArray(1)
       Exit Function

    End If

    CreateUniqueFileName = GetUniqueName(StrPath, FileNameArray, orderId)

End Function


Public Function GetUniqueName(ByRef StrPath As String, ByRef FileNameArray As Variant, ByVal orderId As Integer) As String
' StrPath and FIlenamearray are passed by reference as they don't change during the recursion
' orderid is passed by value so that we don't change the value of orderid in the calling code.
' If this side effect is desired, change the ByVal to ByRef

Dim myFilename                                     As String

    myFilename = FileNameArray(0) & Format(orderId, "0000") & FileNameArray(1)

    If (DoesFileExist(StrPath & myFilename)) Then

        GetUniqueName = GetUniqueName(StrPath, FileNameArray, orderId + 1)

    Else

        GetUniqueName = myFilename

    End If

End Function

Please note that I haven't run the code above but it does compile fine.

good thinking again. Checking for the extension. In this particular case, all the filenames have an extension because the procedure is used for looking through a lot of images. But nevertheless, it is good to check if an extension is included. Thank you for pointing mee to the flaw, sounds logical.

Collectives™ on Stack Overflow

Excel VBA Recursive function return not the expected result

2 Answers 2

6 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related