2
\$\begingroup\$

I often receive large Excel-sheets in which data has been inserted via copy and paste. Usually a lot of the cells contain whitespaces at the beginning and the end before / after the actual value.

I made a VBA-script for to get rid of these whitespaces.

' Removes whitespaces at the
' beginning and the end
' of all cell-values.

Public Sub trimCells()
    Dim lastRow As Long
    Dim lastColumn As Long
    Dim row As Long
    Dim column As Long

    On Error GoTo errorHandler

    ' Find() == VBA-equivalent to
    ' the find-menue in the Excel-GUI.
    ' What:="*" => Matches EVERY content.
    ' Start in the bottom-right corner &
    ' search from there through the rows
    ' upward.
    lastRow = Cells.Find(What:="*", _
                         SearchOrder:=xlByRows, _
                         SearchDirection:=xlPrevious).row
    ' Search in the last row with content for most
    ' right cell with a value.
    lastColumn = Cells(lastRow, Columns.Count).End(xlToLeft).column

    ' Go through the detected range &
    ' apply the trim-function on every cell.
    For row = 1 To lastRow
        For column = 1 To lastColumn
          Cells(row, column).Value = _
            Trim(Cells(row, column).Value)
        Next
    Next

    Exit Sub

errorHandler:
    MsgBox "Error has occurred.", _
      vbCritical, "Error!"

End Sub

I apply the trim-function on every cell-value. So I ask myself:

What is better?

Doing unnecessary executions of the trim-function or checking if it has to be applied? Checking would cost me an additional if-then.

UPDATE

The above algorithm contains an error.

Concerning a table with the following structure:

First | Second |

Alpha | Beta | Gamma |

One |

The result would be:

  • lastRow: 3
  • lastColumn: 1

Reason: It searches in the last row for the most right column. But what is needed is the most right column overall.

Now I determine lastRow and lastColumn this way:

lastRow = Range("A1").SpecialCells(xlCellTypeLastCell).row

lastColumn = Range("A1").SpecialCells(xlCellTypeLastCell).column
\$\endgroup\$
  • \$\begingroup\$ Just a note. If your data table also contains the headers, then you can reliably find the last column simply by finding the (lastcolumn) of the header row. \$\endgroup\$ – Kaz Mar 15 '16 at 9:44
5
\$\begingroup\$

It's faster to just trim a value than it is to try and figure out if it needs trimming in the first place.

However, that's not where your performance bottleneck is.


My personal #1 rule of fast spreadsheet manipulations:

Thou shalt not directly manipulate data in worksheets

By this I mean, doing anything in a worksheet has huge computational overhead. In the VBA object heirarchy, worksheets are only 2 steps removed from the application object itself. There are layers upon layers of abstractions, events, handlers, objects (not to mention several Billion range objects) buried in a worksheet object, and any time you do something in it, it will trigger a cascade of operations to make sure that nothing in your worksheet gets messed up.

Your sub might run fine for a couple of hundred cells in a small worksheet, but a few thousand cells in a larger spreadsheet might suddenly become unbearably slow.

For this reason, you should interact with worksheets as infrequently as possible. If there is data in your worksheet that you need to analyse: access the worksheet once to read the data into an Array, then do all your computations on the Array, then access the worksheet once to read the data back.

Your sub then looks like this:

Public Sub TrimWorksheetCellValues(Optional ByRef targetSheet As Worksheet)

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlCalculationManual

    On Error GoTo errorHandler

    If targetSheet Is Nothing Then Set targetSheet = ActiveSheet '/ If no sheet is provided, assume the active sheet

    Dim firstRow As Long, lastRow As Long
    firstRow = 1
    lastRow = targetSheet.Cells.Find(What:="*", _
                         SearchOrder:=xlByRows, _
                         SearchDirection:=xlPrevious).row

    Dim firstColumn As Long, lastColumn As Long
    firstColumn = 1
    lastColumn = targetSheet.Cells(lastRow, Columns.Count).End(xlToLeft).column

    Dim sheetRange As Range
    Set sheetRange = targetSheet.Range(Cells(firstRow, firstColumn), Cells(lastRow, lastColumn))

    Dim sheetArray As Variant
    sheetArray = Array()
    sheetArray = sheetRange


    Dim ix As Long, iy As Long
    Dim elementValue As Variant, trimmedValue As Variant

    For ix = firstRow To lastRow
        For iy = firstColumn To lastColumn
            elementValue = sheetArray(ix, iy)
            trimmedValue = Trim(elementValue)
            sheetArray(ix, iy) = trimmedValue
        Next iy
    Next ix

    sheetRange = sheetArray

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlCalculationAutomatic

    Exit Sub

errorHandler:
    MsgBox "Error has occurred.", _
      vbCritical, "Error!"

End Sub

A few other things I modified while I was there:


Function names should be written in PascalCase with the leading word capitalised


Things should be explicitly scoped. Your sub was implicitly targeting the active sheet everywhere but nowhere was that explicitly declared or confirmed as the desired functionality. The sub now explicitly targets a particular sheet or explicitly targets the active sheet if no target is provided.


The firstRow and firstColumn are now explicitly declared and assigned, rather than magically appearing halfway through your code.


Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual

These are the standard application settings to turn off if you want your macros to run faster. You should make sure any error handling resets them to their previous values in case your procedure terminates unexpectedly.


Don't catch errors if you're not going to handle them properly. This:

errorHandler:
    MsgBox "Error has occurred.", _
      vbCritical, "Error!"

Is just an error message, but with the useful information (error#, warning etc.) taken out. Do something useful with it (like resetting the application settings) or remove it.

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