HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Remove blank entries from row

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromblankremoverowentries

Problem

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

Solution

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!

Code Snippets

Public Sub removeBlankEntriesFromRow(inputRow As Range, pasteLocation As String)
ByVal inputRow As Range, ByVal pasteLocation As String
Dim oldArray, newArray, tempArray
Dim oldArray
Dim newArray
Dim tempArray
Dim oldArray As Variant()
Dim newArray As Variant()
Dim tempArray As Variant()

Context

StackExchange Code Review Q#86192, answer score: 10

Revisions (0)

No revisions yet.