3
\$\begingroup\$

I am copying data, that is one long list, into an excel sheet and I want to use a macro to organize it visually. The problem is that the data I am copying has empty cells that are unnecessary and will inhibit my code later in the macro and I want to get rid of them. This is what I came up with, but I am hoping there is a more efficient way to do it. The main problem I ran into in trying other methods is I couldn't set a value to a variable to end the second "For" statement before it tried to select a cell that doesn't exist. After it got to the first row it would keep running and would return an error on "ActiveCell.Offset(-1, 0).Range("A1").Select" because there was no cell to select.

Data example:

1, ,2, ,1, ,3, ,1, ,2, ,1, ,4, ,1, ,2, ,1, ,3, ,1, ,2, ,1,...

1, , ,1, , ,2, , ,1, , ,1, , ,2, , ,1, , ,1, , ,3, , ,1, ,...

Each comma represents a new cell, with the numbers and spaces (empty cells) in the same column.

My code:

Sub Organize_Data()

Dim n As Integer
Dim h As Integer
Dim t As Integer
h = 1000
t = h - 1

Cells(h, 1).Select
For n = 1 To t
    If IsEmpty(ActiveCell) Then
    ActiveCell.Offset(-1, 0).Range("A1").Select
    h = h - 1
    Else
    n = t
    End If
Next n

t = h - 1

For n = 1 To t
    If IsEmpty(ActiveCell) Then
    Selection.Delete Shift:=xlUp
    ActiveCell.Offset(-1, 0).Range("A1").Select
    h = h - 1
    Else
    ActiveCell.Offset(-1, 0).Range("A1").Select
    h = h - 1
    End If
Next n

End Sub

Data After Macro:

1,2,1,3,1,2,1,4,1,2,1,3,1,2,1,...

1,1,2,1,1,2,1,1,3,1,...

Possibilities:

I was thinking that in place of the first "For" statement I could use

Selection.End(xlUp).Select

but I don't know how I would set the variable "t" so that the "For" Statement would end before it tried to select a cell that didn't exist. The other thing I can think of is maybe there is code that I could use for a third part to the "If" statement in the second "For" statement that will stop the "For" statement when it is in the first row.

\$\endgroup\$
  • 2
    \$\begingroup\$ Would you explain more clearly what's your actual goal? Is it "packing up" every column by deleting all empty cells in them (as It seems to me) or deleting all rows whose cell in column A is empty (as @Mat'sMug got it) or what? \$\endgroup\$ – user3598756 Aug 25 '16 at 22:09
  • \$\begingroup\$ @Comintern short answers /alternative implementations that are justified with sufficient explanation make perfectly good CR answers ;-) \$\endgroup\$ – Mathieu Guindon Aug 25 '16 at 23:15
  • \$\begingroup\$ Before giving my contribution I'm still waiting for Jacob to answer my question and clarify its real goal. It's actually hard (if not wrong) to attempt at any CR without such a knowledge. @Comintern seems to have had my very same guessing notwithstanding Mat'sMug answer had already been accepted... \$\endgroup\$ – user3598756 Aug 26 '16 at 5:34
3
\$\begingroup\$

You're working off Selection. That's "good enough" for macro-recorder code, which mimicks a user's interactions with a worksheet.

VBA code can do that, sure. But why merely mimick a user's interactions with a worksheet when you have the entire Excel object model in your hands?

But before we look at different/better ways to accomplish what this code does, let's take a look at what it's doing:

  • h is initialized at an arbitrary value of 1000. See this SO answer for a very detailed explanation of various ways to locate the last row that contains data on a worksheet. For example this would set h to the last row with data in column A:

    h = ActiveSheet.Range("A" & ActiveSheet.Rows.Count).End(xlUp).Row
    

    By working off the actual last row, you avoid extraneous iterations later, and avoid having to modify your code if/when the worksheet starts containing more than 1000 rows.

  • We Select the first cell in the 999th row, and then start looping from 1 to 999.

    If the ActiveCell is empty, we move up a row; else, we... wait a minute... we reassign the loop variable back to 999??

    It's pretty hard to understand what's happening here, and more importantly, why - and explaining why something is happening is exactly what comments are for!

    'this loop sets n to the last row with data (right?)
    

Okay, let's stop here, and look again at what we're trying to accomplish.


We want to delete rows where column A is empty. By adapting this answer to Conditionally deleting rows in a worksheet, we can enormously simplify what's going on here - the key being this little function:

Private Function CombineRanges(ByVal source As Range, ByVal toCombine As Range) As Range
    If source Is Nothing Then
        Set CombineRanges = toCombine
    Else
        Set CombineRanges = Union(source, toCombine)
    End If
End Function

Equipped with this simple tool, we can now iterate our rows once, "union" all empty cells, and delete them all at once:

Public Sub CleanUpActiveSheet()

    Dim target As Worksheet
    Set target = ActiveSheet

    Dim lastRow As Long
    lastRow = target.Range("A" & target.Rows.Count).End(xlUp).Row

    Dim toDelete As Range

    Dim currentRow As Long
    For currentRow = 1 To lastRow
        If IsEmpty(target.Cells(currentRow, 1)) Then
            Set toDelete = CombineRanges(toDelete, target.Cells(currentRow, 1))
        End If
    Next

    If Not toDelete Is Nothing Then toDelete.Delete xlUp

End Sub

Notice:

  • Consistent indentation makes it easier to identify where code blocks begin and end.
  • Meaningful variable names make it easier to understand what's going on and why.
  • Variables being declared closer to their usage (as opposed to "at the top of the procedure") makes reading the code more fluid.
  • Working with object references instead of against the current Selection makes the code much more straightforward.
  • Variables meant to hold a row number are declared as Long, because a 64-bit worksheet can have [many] more than 65,535 rows.
  • Procedure names are PascalCase; avoid using underscores in procedure names, VBA uses them for private Object_EventName handlers and Interface_Member implementation procedures.
\$\endgroup\$
  • \$\begingroup\$ "Variables being declared closer to their usage (as opposed to "at the top of the procedure") makes reading the code more fluid." funny, to me it's the opposite! While it should be clearly said that those naming rules aren't compulsoray at all, since VBA doesn't care neither for Camel or Pascal cases nor for underscores in procedure names. They are just a possible convention \$\endgroup\$ – user3598756 Aug 25 '16 at 21:55
  • 2
    \$\begingroup\$ @user3598756 sure. and notice how every single VBA API (e.g. Office/Excel object model, ADODB, etc.) exposes PascalCase public members, and none of these members have underscores in their names. .NET naming guidelines/conventions are about writing code that's consistent with the framework - I don't see why it can't be the same for VBA code. As for where to declare variables.. the smallest scope in VBA is procedure level, so it makes no difference, except I've seen enough wall-of-declarations-at-the-top to cringe at them and be annoyed with constantly scrolling up and down the code to read. \$\endgroup\$ – Mathieu Guindon Aug 25 '16 at 22:02
  • \$\begingroup\$ I noticed everything you said and mostly conform my coding to it. Still they are guidelines, not rules, and so I'd never write something like "Procedure names ARE PascalCase" or the likes. As for declaration code, I find it much more annoying the interruption of a (possibly) fluent code by statements that have nothing to do with the current flux. Just tastes... \$\endgroup\$ – user3598756 Aug 25 '16 at 22:18
  • \$\begingroup\$ @user3598756 FWIW this bullet-list applies to the code snippet I wrote, so yes, in that snippet, procedure names ARE PascalCase. Having worked in a VB6 code base featuring 1500-liner procedures with 2 screens worth of declarations at the top, I'm sorry to say it's a little more than "just tastes" - for me there's no turning back, ever. Makes refactoring /extracting methods much easier, too. But whatever. \$\endgroup\$ – Mathieu Guindon Aug 25 '16 at 22:32
  • 1
    \$\begingroup\$ @user3598756 - Just to toss my 2 cents in, the readability benefit of good naming conventions and putting declarations close to use is a bit lost without a counter-example. Take this code. Quick! What is mtrx used for? A good rule of thumb is to consider how much work it will be to figure it out when it breaks or needs a change in six months (or longer). \$\endgroup\$ – Comintern Aug 25 '16 at 23:37
2
\$\begingroup\$

@Mat'sMug covers the procedure as written very well, although I'd add that using the IsEmpty function to determine whether a cell has something in it is problematic. The function is actually not testing whether a cell is "empty" - it is testing whether the passed Variant is equal to vbEmpty:

Dim foo As Variant
'This...
Debug.Print IsEmpty(foo)    'Prints True
'...is exactly the same as this...
Debug.Print foo = vbEmpty   'Prints True

'But this...
foo = vbNullString
Debug.Print IsEmpty(foo)    'Prints False - foo has a value now.
'...is not the same as this:
Debug.Print foo = vbNullString

The test IsEmpty(ActiveCell) is taking the return value of the default property of ActiveCell, which is ActiveCell.Value (it returns a Variant) and is seeing if it is equal to vbEmpty. This is a good way to run into problems later, because if the cell has a formula in it that evaluates to "", IsEmpty will return False:

Range("A1").Formula = "=TRIM("" "")"
Debug.Print IsEmpty(Range("A1"))  'False

If you get more than one cell in the Range, it will never return True, because .Value will be a Variant containing an array of (empty) Variant's:

Range("A1:A2").ClearContents
Debug.Print IsEmpty(Range("A1:A2")) 'False

Don't become an IsEmpty casualty. Determine what your business rule is for an "empty" cell, and test that instead. In your case, it would probably be ActiveCell <> vbNullString.


That out of the way, you should try to use the tools that the host application offers when possible (and performance allows). Excel actually keeps track of blank cells in its SpecialCells collection. You can identify all of them by calling .SpecialCells(xlCellTypeBlanks), which replaces the need for manually identifying them cell by cell. The .Delete method also works on non-contiguous cells, so the entire Sub can basically just be replaced with a one-liner:

Sub Organize_Data()
    ActiveSheet.UsedRange.SpecialCells(xlCellTypeBlanks).Delete Shift:=xlUp
End Sub
\$\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.