2
\$\begingroup\$

I am using VBA in Excel to remove the word NULL from cells. I have the following code:

Sub RemoveNull()
    Dim c, count, r
    Application.ScreenUpdating = False

    count = 0
    r = ActiveCell.row
    c = ActiveCell.Column
    c = GetLetterFromNumber(c)

    For x = 1 To Range(c & Rows.count).End(xlUp).row Step 1
        count = count + 1
        If Range(c & x).Value = "NULL" Then
            Range(c & x).Clear
        End If
        'This section is for trouble shooting
        'If count = 1000 Then
        '    Range(c & x).Select
        '    count = 1
        'End If
    Next
    Application.ScreenUpdating = True
    MsgBox "Finished"
    Range(c & r).Select
End Sub

This works just fine and is designed to only check one column. I don't want to check the whole sheet with this I already have another sub that does this.

I want to make sure that this is the most efficient way to go about it. right now it checks each cell from top to bottom and then clears the contents if it contains NULL.

\$\endgroup\$
3
\$\begingroup\$

Static Code Analysis

I took your code and ran it through Rubberduck (an open-source VBIDE add-in project I manage) code inspections - here's what came out of it:

  • Implicit references to ActiveSheet

    When they're not qualified with an actual Worksheet object instance, members Range and Rows (and Columns, Names and Cells) implicitly refer to the ActiveSheet. These implicit references make the code frail, and harder to debug. Consider making these references explicit when they're intended (e.g. ActiveSheet.Range instead of just Range; ActiveSheet.Rows instead of just Rows), and prefer working off object references (e.g. DataSheet.Range, DataSheet.Rows).

  • Procedure RemoveNull is implicitly Public

    Module members are public by default, which can be counter-intuitive (especially if you're coming from other programming languages). Consider specifying explicit access modifiers to avoid ambiguity. e.g. Public Sub RemoveNull() instead of just Sub RemoveNull().

  • Instruction contains multiple declarations

    Dim c, count, r
    

    Declaring multiple variables in the same instruction is legal, but should be used sparingly. Consider declaring variables closer to their usage, in a single instruction per declaration.

  • Implicit Variant variables

    Variables c, r, and count are all implicitly Variant, because they're declared without an As clause. Consider specifying an appropriate data type for all variables (e.g. String for strings, Long for row numbers, etc.), and use an explicit Variant where Variant is intended. Note that there's a common beginner trap here:

    Dim c, count, r As Long ' only r is a Long, c and count are both Variant
    
  • Use meaningful names

    c, r, and x are poor names. Prefer using names that convey usage and meaning, e.g. activeColumn, activeRow, and currentRow. Avoid disemvoweling, numeric suffixes, Systems Hungarian prefixes and 1-2 character identifiers.

  • Undeclared variables

    Variable x isn't declared, which means it's an implicit Variant, declared on-the-fly by the VBA runtime. If your code can compile and run, it means Option Explicit isn't specified either.

  • Option Explicit not specified

    VBA will happily compile a typo: use Option Explicit to prevent successfully compiling a program that doesn't explicitly declare every single variable it's using.

Future versions of Rubberduck will also highlight that Step 1 is the default step for a For loop, and therefore redundant.


Redundancies

Static code analysis is nice, but can't pick up everything. Like this:

c = ActiveCell.Column
c = GetLetterFromNumber(c)

You have the Column number already - there's absolutely no need to work out what column "letter" that number is referring to. Simply change how you're accessing the cell - instead of this:

[ActiveSheet.]Range(c & x).Clear

Use Cells:

[ActiveSheet.]Cells(x, c).Clear

Notice how much easier that is to parse, with meaningful names:

[ActiveSheet.]Cells(currentRow, activeColumn).Clear

So, remove GetLetterFromNumber, you don't need it at all. Ever.

Static code analysis also won't pick up (for now, anyway) that Range.Value has a non-zero possibility of containing an error value - in which case a comparison with any non-error type will raise run-time 13 / Type Mismatch:

If Range(c & x).Value = "NULL" Then ' possible error here

The loop body is pulling the same cell twice, and in order to validate whether it's an error or not, you'd need to pull it a third time: consider extracting a local variable.

Dim currentCell As Range
Set currentCell = [ActiveSheet.]Cells(x, c)

If Not IsError(currentCell.Value) Then
    If currentCell.Value = "NULL" Then ' comparison is now safe
        currentCell.Clear
    End If
End If

The last thing you want is the possibility of throwing a run-time error *while Application.ScreenUpdating is turned off. For this reason, whenever you need to toggle that setting, consider introducing some minimal error handling:

Public Sub DoSomething()
    On Error GoTo CleanFail
    Application.ScreenUpdating = False

    'code...

CleanExit:
    Application.ScreenUpdating = True
    Exit Sub
CleanFail:
    Debug.Print Err.Description
    Stop ' we don't know what happened; break here and debug
    Resume CleanExit
End Sub

Performance

Range is an object collection - the fastest way (by a lot) to iterate it is with a For Each loop. But, if all you need out of a Range its its Value, you can iterate the values even faster by dumping the range into an array, and iterating the array with a For...Next loop:

With ActiveSheet

    Dim lastRow As Long
    lastRow = .Cells(.Rows.count, c).End(xlUp).Row

    Dim values As Variant ' a variant array
    values = .Range(.Cells(1, 1), .Cells(.Rows.Count, lastRow)).Value

End With

There are plenty of Stack Overflow Q&A's about the best ways of turning that 2D array into a 1D array, I'll leave that bit of research up to you.

Once you have a one-dimensional array, you can iterate and modify it in-memory:

For x = LBound(values) To UBound(values)
    If Not IsError(values(x)) Then
        If values(x) = "NULL" Then values(x) = vbEmpty
    End If
Next

And then you can convert back the 1D array to a 2D array, and dump it onto the worksheet in one single write:

ActiveSheet.Cells(1, activeColumn).Resize(UBound(values)) = Application.Transpose(values)

And with one single read and one single write from the worksheet, and the iteration and actual work being done in-memory at lightning speed (as opposed to one cell at a time with Excel repainting itself every time, if not recalculating the whole worksheet too), there shouldn't even be a need to switch off Application.ScreenUpdating now - toggling it off won't make a difference anyway.

I suspect the result will be so fast, you won't even need that MsgBox call to wake the user up.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for the thoroughness of your response. I have implemented most of your suggestions and as stated it works almost instantly even for 65k+ rows. I do have a follow-up question though. What is the purpose or best practice for diming the variables in different places instead all at once in a block at the top or even in one line? \$\endgroup\$ – Mike Sep 26 '17 at 18:52
  • \$\begingroup\$ Code should be written to be read, not just executed. Wall-of-declarations style is often brought up to defend [Systems] Hungarian Notation - "that way I know the type of the variables where I use them", vs. declare them where you use them, and use meaningful names; no need for a "type" prefix when the declared type is in your face. Also reduces cognitive load (especially with long procedures) and pretty much eliminates the possibility for declaring a variable that you're not using. \$\endgroup\$ – Mathieu Guindon Sep 26 '17 at 18:58
  • \$\begingroup\$ Basically, for short procedures it probably doesn't matter. For everything else, a wall-of-declarations is a maintenance burden. \$\endgroup\$ – Mathieu Guindon Sep 26 '17 at 19:01
  • \$\begingroup\$ Makes perfect sense, though I've never thought about it. Again thanks for the help it is much appreciated! \$\endgroup\$ – Mike Sep 26 '17 at 19:24
  • \$\begingroup\$ @Mike no problem! And give Rubberduck a star on GitHub if you like! =) \$\endgroup\$ – Mathieu Guindon Sep 26 '17 at 19:25
0
\$\begingroup\$

Regarding OP - everything Mat said

I think Range.Replace is more efficient in this case - faster (and less code)


Option Explicit

Public Sub RemoveNull()
   With ThisWorkbook.ActiveSheet.UsedRange
      With .Columns(ActiveCell.Column - .Column + 1)
         .Replace What:="NULL", Replacement:=vbNullString, LookAt:=xlWhole, MatchCase:=False
      End With
   End With
End Sub
|improve this answer|||||
\$\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.