Remove blank entries from row

Question

I wrote a sub to remove the blank entries in a row without shifting the cells around but it seems unnecessarily clunky and I'd like to get some advice on how to improve it.

Public Sub removeBlankEntriesFromRow(inputRow As Range, pasteLocation As String)
    'Removes blank entries from inputRow and pastes the result into a row starting at cell pasteLocation

    Dim oldArray, newArray, tempArray
    Dim j As Integer
    Dim i As Integer

    'dump range into temp array
    tempArray = inputRow.Value
    'redim the 1d array
    ReDim oldArray(1 To UBound(tempArray, 2))
    'convert from 2d to 1d
    For i = 1 To UBound(oldArray, 1)
        oldArray(i) = tempArray(1, i)
    Next
    'redim the newArray
    ReDim newArray(LBound(oldArray) To UBound(oldArray))
    'for each not blank in oldarray, fill into newArray
    For i = LBound(oldArray) To UBound(oldArray)
        If oldArray(i) <> "" Then
            j = j + 1
            newArray(j) = oldArray(i)
        End If
    Next
    'Catch Error
    If j <> 0 Then
        'redim the newarray to the correct size.
        ReDim Preserve newArray(LBound(oldArray) To j)
        'clear the old row
        inputRow.ClearContents
        'paste the array into a row starting at pasteLocation
        Range(pasteLocation).Resize(1, j - LBound(newArray) + 1) = (newArray)
    End If
End Sub

Answer 1

Let's start with the procedure's signature:

Public Sub removeBlankEntriesFromRow(inputRow As Range, pasteLocation As String)

I like that you're explicitly making it Public - VBA procedures are Public by default, but in most other languages members are private if an access modifier isn't specified, which can make reading VBA code a bit confusing if you don't know this and you're switching languages (VB.NET, C#, etc.). Even if the only language you code in is VBA, being explicit about access modifiers is a very good practice, keep it! :)

I also like that your procedure has a meaningful name that's starting with a verb - procedures do something, and they should be doing something very specific. Giving a specific name to your procedures makes it easier to see what's doing what. By convention, removeBlankEntriesFromRow should be RemoveBlankEntriesFromRow though, as PascalCase is the recommended casing style for pretty much everything except local variables and parameters.

Parameters can be passed by value (i.e. method receives its own copy) or by reference (i.e. reassigning a parameter will affect the caller's copy too) - in other languages parameters are often passed by value by default, but VBA passes them ByRef unless you explicitly specify they're ByVal.

Hence, if you don't intend to reassign a parameter and have the calling code "see" that new value, you should always pass parameters by value, like this:

ByVal inputRow As Range, ByVal pasteLocation As String

---

Dim oldArray, newArray, tempArray

For better readability and maintainability, it's best to declare the 3 variables on 3 separate instructions:

Dim oldArray
Dim newArray
Dim tempArray

Now, these 3 variables are all implicitly Variant. If you intend them to be Variant, it's best to say so - and since a Variant can be just about anything in VBA, it will happily hold an array of variants - better declare them as their exact type - Variant():

Dim oldArray As Variant()
Dim newArray As Variant()
Dim tempArray As Variant()

And now we know what we're up against! :)

---

Dim j As Integer
Dim i As Integer

Typical loop counters, everybody calls 'em that. But it's probably better to name them after their usage, too - and Integer will probably fit most usages, but the day you need to run this on a range that has more than 32,767 cells it will overflow the 16-bit Integer type; I'd use a 32-bit Long here, just to be safe:

Dim oldIndex As Long
Dim newIndex As Long

---

If oldArray(i) <> "" Then

This is a minor point, but I have to mention it: the hard-coded "" string literal may look like it's just an empty string, but it's actually allocated a memory space of its own. Nothing to worry about, but it's usually better to use the built-in constant vbNullString instead.

Try this in the immediate pane (Ctrl+G):

?StrPtr(""), StrPtr(vbNullString)

You should see a bunch of numbers (a memory address) for the StrPtr("") call, and a 0 for the StrPtr(vbNullString) call: vbNullString is completely zero-footprint, give it the love it deserves!

---

'Catch Error

This comment is misleading: you're not handling any runtime errors that may occur here. To handle errors, your procedure body should be templated something like this:

Public Sub DoSomething()
    On Error GoTo CleanFail

    'method body goes here

CleanExit:
    'cleanup code goes here
    Exit Sub

CleanFail:
    'error-handling code goes here
    Resume CleanExit
End Sub

That said, I would remove all but the first comment. Comments shouldn't say what the code is doing - that's the code's job (and that's why code readability matters). Good comments say why the code is doing something.

Wherever you have a comment that says "this chunk of code does X", then you might have an opportunity to name that chunk of code, by extracting it into its own function. That can be done manually of course, but the Rubberduck VBE add-in I've written with @RubberDuck makes it easier than ever: just select a block of code, and then refactor / extract method and you're done!

Answer 2

Here is my answer from SO:

I would do it by converting the array to a string joined with pipe |, clean any double pipes out (loop this until there are no doubles left) then push it back to an array across the row:

Here is my code:

Sub TestRemoveBlanks()
    RemoveBlanks Range("A1")
End Sub

Sub RemoveBlanks(Target As Range)
    Dim MyString As String
    MyString = Join(WorksheetFunction.Transpose(WorksheetFunction.Transpose(Range(Target.Row & ":" & Target.Row))), "|")
    Do Until Len(MyString) = Len(Clean(MyString))
        MyString = Clean(MyString)
    Loop
    Rows(Target.Row).ClearContents
    Target.Resize(1, Len(MyString) - Len(Replace(MyString, "|", ""))).Formula = Split(MyString, "|")
End Sub

Function Clean(MyStr As String)
    Clean = Replace(MyStr, "||", "|")
End Function

I put a sub to test in there for you.

If you have pipes in your data, substitute it with something else in my code.

Answer 3

I suppose I might as well chime in with my post from SO too, not really a review of the code in the question, but a different take with some explanation:

Here is my take on the task you describe:

Option Explicit
Option Base 0

Public Sub removeBlankEntriesFromRow(inputRow As Range, pasteLocation As String)
    'Removes blank entries from inputRow and pastes the result into a row starting at cell pasteLocation
    Dim c As Range
    Dim i As Long
    Dim new_array As String(inputRow.Cells.Count - WorksheetFunction.CountBlank(inputRow))

    For Each c In inputRow
        If c.Value <> vbNullString Then
            inputRow(i) = c.Value
            i = i + 1
        End If
    Next

    Range(pasteLocation).Resize(1, i - 1) = (new_array)
End Sub

You'll notice that it is quite different, and while it may be slightly slower than your solution, because it is using a for each-loop instead of looping through an array, if my reading of this answer is correct, it shouldn't matter all that much unless the input-range is very large.

It is significantly shorter, as you see, and I find it easier to read - that may just be familiarity with this syntax as opposed to yours though. Unfortunately I'm not on my work-computer atm. to test it out, but I think it should do what you want.

If your main objective is to improve the performance of the code, I think that looking into what settings you may turn off while the code is running will have more effect than exactly what kind of loop and variable assignment you use. I have found this blog to be a good introduction to some concepts to bear in mind while coding in VBA.

I hope you have found my take on your problem an interesting comparison to your own solution, which as others have mentioned should work just fine!