5
\$\begingroup\$

I am using the following VBA code to replace accented letters with regular letters in a spreadsheet. This is necessary because these spreadsheets have to be uploaded to an import tool that does not allow foreign characters.

Function RemoveAccentsFromForeignLetters()

    StartNewTask ("Removing accents from foreign letters")

    Dim AccChars As String
    Dim RegChars As String

    AccChars = "ŠŽšžŸÀÁÂÃÄÅÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖÙÚÛÜÝàáâãäåçèéêëìíîïðñòóôõöùúûüýÿ"
    RegChars = "SZszYAAAAAACEEEEIIIIDNOOOOOUUUUYaaaaaaceeeeiiiidnooooouuuuyy"
    Set MyRange = ActiveSheet.UsedRange

    Dim A As String * 1
    Dim B As String * 1
    Dim i As Integer

    For i = 1 To Len(AccChars)
        A = Mid(AccChars, i, 1)
        B = Mid(RegChars, i, 1)
        MyRange.Replace What:=A, Replacement:=B, LookAt:=xlPart, MatchCase:=True
        ' TODO: highlight changed cells yellow
    Next

End Function

I googled the code from somewhere and it works, but it is a bit slow. In a spreadsheet with 1.5 million cells (7000 rows, 200 columns), it takes 21 seconds to run.

I wanted to look into ways to optimize it, for example:

  • Maybe a RegEx would be faster?
  • Maybe I should pass the entire spreadsheet to a DLL using an array, have the DLL do the replace, then pass it back? One article I read suggested that this is up to 10x faster than using native Excel VBA.
  • Maybe I should use the same DLL trick as above, but add multi-threading?
  • Any other ideas?
\$\endgroup\$
4
\$\begingroup\$

In addition to @Raystafarian's observations, there are a couple of other issues that I see.

  1. I would personally put your AccChars and RegChars variables into Consts, because you never change their values.
  2. Your code also requires that AccChars and RegChars are the same length, and will fail if they aren't. I'd add an assert that tests that before you do anything that writes to the Worksheet.
  3. You generally want to avoid using the Integer type unless you absolutely need to (for example in an API call). VBA stores them as a Long regardless of how they are declared.
  4. Declaring A and B as fixed-length strings is a bit of premature optimization that is backfiring. When you pass them to .Replace as parameters, they are actually being implicitly cast back to variable length ones.
  5. Minor thing, but I prefer the term "stripped" to "regular" - all characters are "regular" in their native context.
  6. Using string functions is not ideal when what you really care about are individual characters as opposed to a sub-string. VBA allows a direct assignment of a String to a Byte array, and indexing into the array is much faster than calling Mid. The performance hit is a lot higher when you're doing it inside a loop. As a side note, you should always use the String returning functions that end with '$' to avoid superfluous casting unless you explicitly require a Variant type. Mid$ (returns a String) as opposed to Mid (returns a Variant).
  7. Your call to .Replace method is acting on every single cell in your Range. This is a huge performance hit, because I'm guessing that not every cell in the entire Worksheet is going to have an accented character in it. You should really only be concerned with cells that do. By performing the replacement on every cell, your performance is scaling directly with the number of cells, not the number of replacements. So, if only 5% of the cells have accented characters you are still doing 100% of the work. This is where the regular expression would be useful, but you can't easily pawn that off on Excel (except maybe with using .Find, which has issues of its own). A loop would be better - a loop over an array pulled from the Range would be best.
  8. Your "TODO: highlight changed cells yellow" is going to be much more difficult using the .Replace function, because it would require storing the state of the entire sheet, then doing a cell-by-cell comparison. It will be a lot easier to track this concurrently while you are making changes.

With all of that in mind, I'd do something more like this:

Private Sub RemoveAccentsFromForeignLetters()
    Dim Target As Range
    Set Target = ActiveSheet.UsedRange

    StartNewTask ("Removing accents from foreign letters")

    Dim Values() As Variant
    Values = Target.Value

    Debug.Assert Len(AccentedChars) = Len(StrippedChars)

    Dim FindChars() As Byte
    Dim ReplaceChars() As Byte
    FindChars = AccentedChars
    ReplaceChars = StrippedChars

    Dim AccentedTest As RegExp
    Set AccentedTest = New RegExp
    AccentedTest.Pattern = "[" & AccentedChars & "]"

    Dim index As Long
    Dim character As Long

    Dim col As Long
    Dim row As Long
    For row = 1 To UBound(Values, 1)
        For col = 1 To UBound(Values, 2)
            'Ignore strings that don't require character replacements.
            If AccentedTest.Test(Values(row, col)) Then
                Dim buffer() As Byte
                buffer = StrConv(Values(row, col), vbUnicode)
                'Skip every other character - VBA "Unicode" expansion 
                'inserts nulls there.
                For character = 0 To UBound(buffer) Step 2
                    For index = 0 To UBound(FindChars)
                        If buffer(character) = FindChars(index) Then
                            buffer(character) = ReplaceChars(index)
                        End If
                    Next index
                Next character
                'Highlight changed cells yellow left as an exercise for
                'the reader.
                Values(row, col) = StrConv(buffer, vbFromUnicode)
            End If
        Next col
    Next row

    ActiveSheet.UsedRange = Values
End Sub

Some quick and dirty benchmarks, all done with 2000 rows and 10 columns. In the "worse case" benchmarks, all cells have the value "ŸÀÁÂÃÄÅÇÈÉÊËÌÍÎ". In the "average case" benchmarks, 5% of the cells have the "ŸÀÁÂÃÄÅÇÈÉÊËÌÍÎ" and the rest of them contain "XXXXXXXXXXXXXXX".

Replace method, worst case: 3.51 seconds. 
Array method, worst case: 1.15 seconds. 
Replace method, average case: .40 seconds (supports disabling ScreenUpdating). 
Array method, average case: .08 seconds.
\$\endgroup\$
  • 1
    \$\begingroup\$ WOW. Execution time on my benchmark file with 1.5 million cells went from 20.62 seconds to 1.61 seconds. Much faster code! And you're saying that, for some reason, MyRange.Replace changes every cell in the workbook? So your idea of converting the sheet to a Range variable, making the changes to the variable, then writing the variable back to the sheet is much faster? Looking forward to your response. I want to wrap my head around this so that I can optimize other functions :-) \$\endgroup\$ – AdmiralAdama Mar 28 '16 at 2:25
  • \$\begingroup\$ @AdmiralAdama - Writing values to cells is a deceptively expensive procedure - think about everything Excel has to do. It has to check for formula references to the updated cell, if it finds any it has to update those cells, it has to properly format the value, test for conditional expressions, redraw the screen, etc. You can actually watch it if you have ScreenUpdating active. You'll see it repeatedly writing the cell values back and then redrawing. \$\endgroup\$ – Comintern Mar 28 '16 at 2:32
  • \$\begingroup\$ @AdmiralAdama - I should clarify. MyRange.Replace operates on every cell in MyRange. It only changes cells that have text that is replaced. \$\endgroup\$ – Comintern Mar 28 '16 at 2:36
  • \$\begingroup\$ Huh, I wasn't aware of #6. Nice \$\endgroup\$ – Raystafarian Mar 28 '16 at 7:37
  • \$\begingroup\$ Yes. Doing anything to a spreadsheet incurs enormous computational overhead. Every single time. Setting one Range and writing to an array and then writing back at the end is only 2 spreadsheet operations. Hence why it is *so* much faster. \$\endgroup\$ – Kaz Mar 30 '16 at 15:29
2
\$\begingroup\$

I'm not sure why this is a function rather than a sub - you're doing something, not returning something.

I don't see you dimensioning myRange. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

Another good way to speed up your macro is to use Application.Screenupdating = False and Application.Calculation = xlManual and Application.EnableEvents = False. Just be sure to return them to True and xlAutomatic and True before exiting the sub.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. 1) I chose function so that I would not clutter the macros list. Every sub shows up in the macros list. Should I use Private Sub instead of Function? 2) What modifications should I make to the myRange code to get around the dimensioning/performance issue you mentioned? 3) I tried turning off ScreenUpdating, Calculation, and EnableEvents and the code still took the exact same amount of time (20.79 seconds) to execute on my test document. \$\endgroup\$ – AdmiralAdama Mar 25 '16 at 19:43
  • 1
    \$\begingroup\$ Yeah you could make it private. You just need to dim myRange as __ \$\endgroup\$ – Raystafarian Mar 25 '16 at 22:53

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.