6
\$\begingroup\$

This is a fun one! I wrote some code to automatically get the next revision letter for a file by looking at the existing letter. It works, but I think there is room for improvement as far as simplifying it or making more efficient. Our revision scheme follows ASME Y14.35, which specifies the following rules:

  • First release is revision "-".
  • First revision to a released document is "A", and subsequent revisions increment by one letter.
  • Skip letters "I", "O", "Q", "S", "X", "Z"
  • After rev "Y" the next rev is "AA", and then (following the same rules still) the second letter increments for each revision (e.g. "AB", "AC", etc.) until "AY" is reached, then it goes to "BA", then "BB", etc.
  • "YYY" is the maximum and feasibly should never be reached.

One caveat is that I'm working with some legacy data, and in the past there was no consistency in how the "rev" field was filled out in the database I pull the current rev from. As a result, sometimes for a rev "-" file, the rev field will contain a "-" or a space " " or a zero-length string "" or tilde "~", etc. So the decision was made to handle all non-letters like a rev "-".

Option Explicit
'***NOTE: _
    This module requires the following references: _
        <NONE>

Function NextRevLetter(strCurrentRev As String) As String
'This function returns the next revision letter given an existing revision letter.

'Declare variables
    Dim lngX As Long
    Dim strX As String
    Dim strY As String
    Dim strZ As String

'First, check if we are dealing with rev A-Z or AA-ZZ
        If Len(strCurrentRev) < 2 Then
                If Len(strCurrentRev) = 0 Then strCurrentRev = "-"
                If Asc(strCurrentRev) >= 65 And Asc(strCurrentRev) < 90 Then     'Considers only Letters A - Y.
                    lngX = Asc(strCurrentRev)   'Get the ASCII code for the letter
                        Select Case lngX
                            Case 72, 78, 80, 82, 87    'Skip letters "I", "O", "Q", "S", "X"
                                lngX = lngX + 2
                            Case Else   'For everything else, we just increment by 1 letter
                                lngX = lngX + 1
                        End Select
                        'Check if we have exceeded letter Y
                            If lngX > 89 Then
                                'Next rev is AA.  Return the updated revision and exit function
                                    NextRevLetter = "AA"
                                    Exit Function
                            End If
                Else
                'If we received a non letter, then the rev will be A (e.g. recieved "-")
                    lngX = 65
                End If
        Else
        'Get the next letter for the last letter in the current rev (e.g. rev AB we want "C")
            strX = NextRevLetter(Right(strCurrentRev, 1))
            strY = Left(strCurrentRev, Len(strCurrentRev) - 1)
            'If the last letter was Y, then we need to increment the first letter
                If Len(strX) > 1 Then
                    strZ = Mid(strCurrentRev, Len(strCurrentRev) - 1, 1)
                        If Len(NextRevLetter(strZ)) > 1 Then
                            If Len(strY) > 1 Then
                                strZ = Mid(strY, Len(strY) - 1, 1)
                            Else
                                strZ = Right(strY, 1)
                            End If

                            strZ = NextRevLetter(strZ)
                                If Len(strZ) > 1 Then strZ = Left(strZ, 1)
                                If Len(strY) > 2 Then
                                    strY = Left(strY, Len(strY) - 2)
                                Else
                                    strY = ""
                                End If
                            strY = strY & strZ & "A"
                            strX = "A"
                        Else
                            strZ = NextRevLetter(strZ)
                            strY = Left(strY, Len(strY) - 1) & strZ
                            strX = "A"
                        End If
                End If
                'Return the update revision and exit function
                    NextRevLetter = strY & strX
                    Exit Function
            Exit Function
        End If
    NextRevLetter = Chr(lngX)
End Function
\$\endgroup\$
1
  • \$\begingroup\$ What's the purpose of the NextRevLetter = Chr(…) all the way at the end? \$\endgroup\$ Commented May 14, 2015 at 19:38

2 Answers 2

11
\$\begingroup\$

The variable names lngX, strX, strY, and strZ are 75% Hungarian and 25% useless. I don't have a good idea of what their purpose is. You don't have to write a comment telling me that you are declaring variables; the Dim keyword is obvious enough.

You've used recursion to solve this problem, which makes it a lot more complicated than it needs to be. A For loop that considers each character from right to left would be simpler.

Due to the six excluded letters, using ASCII codes to get the next letter is not as useful a strategy. A lookup table (alphabet in the solution below) should simplify the code. As a bonus, since InStr() for any unrecognized character would return 0, such garbage characters would map to "A". And, as it turns out, this function also happens to correctly handle "" as input, due to the way the carrying mechanism works.

Public Function NextRevLetter(currentRev As String) As String
    ' Alphabet, skipping I O Q S X Z
    Const alphabet As String = "ABCDEFGHJKLMNPRTUVWY"

    NextRevLetter = currentRev
    Dim i As Long
    For i = Len(NextRevLetter) To 1 Step -1
        Dim digit As String
        digit = Mid$(NextRevLetter, i, 1)

        ' Increment digit: "A"->"B", "B"->"C", ..., "H"->"J", ..., "Y"->"".
        ' Any other garbage maps to "A".
        digit = Mid$(alphabet, InStr(alphabet, digit) + 1, 1)

        If Len(digit) = 0 Then
            ' "Y"->"" case. Reset to "A", then carry.
            Mid$(NextRevLetter, i, 1) = "A"
        Else
            Mid$(NextRevLetter, i, 1) = digit
            Exit Function
        End If
    Next
    ' Carry.
    NextRevLetter = "A" & NextRevLetter
End Function
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Relevant: a string is an array of bytes \$\endgroup\$ Commented May 14, 2015 at 19:56
  • \$\begingroup\$ @200_success This is waaaaay simpler and executes a lot faster than my original code. The InStr() approach using a match string is simple and direct. \$\endgroup\$
    – CBRF23
    Commented May 14, 2015 at 20:23
  • \$\begingroup\$ I made some edits to fix some syntax errors with variable declaration in-line and I implemented the explicit string returning mid$() that @Mat'sMug recommended in his answer as well as explicitly declaring the function public. \$\endgroup\$
    – CBRF23
    Commented May 14, 2015 at 20:28
6
\$\begingroup\$

Before anything, kudos for specifying Option Explicit.


Left, Right, Mid, Chr

All these functions return a Variant, and they all have a String-returning little brother that you should be using instead: Left$, Right$, Mid$, and Chr$. Spares an implicit conversion.


Inspector Rubberduck

I ran code inspections, and got 1 single result - that's a very good sign:

Rubberduck Code Inspections
1 issue found.
Suggestion: Member 'NextRevLetter' is implicitly Public - VBAProject.Module1, line 3

Module members are Public unless explicitly specified otherwise; VBA does a lot of things implicitly and this can make things confusing - best be explicit everywhere you can, and make it an explicit Public Function.


Comments

Most comments are redundant and say what the code does instead of why it's doing it. Code should be clear enough to not need comments... but your hard-coding of ASCII codes throughout the code hardly makes the code easy to read and to follow.. let alone the convoluted recursive logic.


I was going to suggest another approach, but @200_success has one that's hard to beat.

I suggest you take a look at this Stack Overflow answer and try to implement @200's logic treating the string as a byte array instead, for optimal performance.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks Mat's Mug - I will play with the byte iteration eventually, but @200_Success's code already executes WAY faster than my code and is easier to read to boot. One consideration for the byte iteration is that even though it may be faster, for me it's a lot easier to read the string matching method above. \$\endgroup\$
    – CBRF23
    Commented May 14, 2015 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.