6
\$\begingroup\$

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
| improve this question | | | | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! I predict you'll be coming back for more once you see the kind of answers we give over here :) \$\endgroup\$ – Mathieu Guindon Apr 7 '15 at 20:45
  • \$\begingroup\$ Welcome to Code Review. If you cross-post from Stack Overflow, please declare it on both sites. \$\endgroup\$ – 200_success Apr 7 '15 at 22:02
  • 1
    \$\begingroup\$ Not really enough for a full post, but you need to protect against invalid input if you are taking an address as a string. removeBlankEntriesFromRow ActiveSheet.Rows(1), vbNullString for example... \$\endgroup\$ – Comintern Apr 8 '15 at 1:23
10
\$\begingroup\$

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!

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the indepth response Mat's Mug. Shows that I have a long way to go to become a proficient VBA programmer. \$\endgroup\$ – learningAsIGo Apr 8 '15 at 21:51
  • \$\begingroup\$ I have some questions on the information in your post: Is there anywhere I can look up more information on the PascalCase? I'm a little confused with the difference between ByVal and ByRef. If I pass a int variable to a function ByRef and change it within the function, then it will change globally outside the function as well even if I don't explicitly return the variable with the function? Good point on the "Catch Error" comment. I meant error here as the sub was erroring if I passed it an entirely blank row but I see that error has a more specific meaning in coding context. \$\endgroup\$ – learningAsIGo Apr 8 '15 at 21:58
  • \$\begingroup\$ Comment cont. I guess I have a long way to go since everything I commented wasn't immediately obvious to me but I guess that will come with time. Is it obvious why three arrays were needed in this sub? I found the coverting from 2D to 1D part confusing. \$\endgroup\$ – learningAsIGo Apr 8 '15 at 21:58
  • \$\begingroup\$ @learningAsIGo indeed, ByRef parameters are a (not quite recommended) way of making a function return multiple values; Rubberduck inspections will flag a parameter as "can be passed by value" whenever it's not assigned in the scope of a procedure, however you should know that ByVal makes a copy of the argument, so if you're passing an object variable it's often better to pass it ByRef for performance reasons - as with everything else, it's a compromise. Don't worry if you don't quite "grok" ByRef/ByVal yet, it's normal to struggle with that as a beginner :) \$\endgroup\$ – Mathieu Guindon Apr 8 '15 at 23:30
  • \$\begingroup\$ Also, I purposely left algorithm-related stuff to other reviewers; feel free to upvote all useful answers, and to accept whichever - don't hesitate to post more of your working code on this site anytime, you'll learn quickly with this community! \$\endgroup\$ – Mathieu Guindon Apr 8 '15 at 23:33
4
\$\begingroup\$

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.

| improve this answer | | | | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! An interesting alternative, but not quite a review of OP's code. CR and SO are different; there's nothing wrong with providing alternatives or even rewriting OP's code, but good CR answers explain how the approach is different and why it's better or more efficient to do so. Also I wouldn't recommend using an explicit Call statement as it's an obsolete construct that's only in the language for backward compatibility with older versions of the language. \$\endgroup\$ – Mathieu Guindon Apr 7 '15 at 23:50
  • 1
    \$\begingroup\$ Thanks for the comment. I would suggest this as an alternative as string manipulation will be much faster than looping through every element of an array. The approach is detailed in the original post (using conversion to string with pipe) \$\endgroup\$ – Dan Donoghue Apr 7 '15 at 23:51
  • \$\begingroup\$ I have removed the Call statement, I never use the Call statemenet but I got hung up on passing a range in and was playing around with Syntax (turned out to be a typo that was killing me) \$\endgroup\$ – Dan Donoghue Apr 7 '15 at 23:58
  • \$\begingroup\$ Reading through my code, we could speed this up further. My code currently converts the whole row to a pipe separated string which results in a bunch of pipes on the end. This causes a number of loops later. It would be better to determine the final row of data and post only that range to the string. Let me know if you are going to go with my code and I will make the changes for you. \$\endgroup\$ – Dan Donoghue Apr 8 '15 at 3:13
2
\$\begingroup\$

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!

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