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
When they're not qualified with an actual
Worksheet object instance, members
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
ActiveSheet.Rows instead of just
Rows), and prefer working off object references (e.g.
RemoveNull is implicitly
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
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.
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 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
x are poor names. Prefer using names that convey usage and meaning, e.g.
currentRow. Avoid disemvoweling, numeric suffixes, Systems Hungarian prefixes and 1-2 character identifiers.
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.
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
Notice how much easier that is to parse, with meaningful names:
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
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
Application.ScreenUpdating = True
Stop ' we don't know what happened; break here and debug
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
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
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
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.