Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a script which I use to delete rows from a Excel Sheet, dependent on the Column C cell of that row being blank.

I believe deleting a range might be quicker than deleting individual rows, but I'm unsure.

Sub Shorter()

'47ms to remove blanks rows of 80 rows

'Searches the designated Column and checks for a Blank Cell
'It then Sets a Marker and then continues until it finds a cell with data in it.
'That blank range is then deleted.

    t = GetTickCount
    With ActiveSheet

        Firstrow = 20
        LastRow = .UsedRange.Rows(.UsedRange.Rows.Count).Row

        For i = LastRow To Firstrow Step -1

            If i = StartRow - 1 Then GoTo EndSubAutoDeleteRowsFromEndRow
            If .Cells(i, "C") = Empty Then

                DelRangeStartRow = i

                ii = 0

                Do While .Cells(i - ii, "C") = Empty
                    DelRangeEndRow = (i - ii)
                    ii = ii + 1
                Loop

                Range(.Rows(DelRangeStartRow), .Rows(DelRangeEndRow)).Delete Shift:=xlUp

            End If
        Next i
    End With
    'Come here if you are finished deleting

    EndSubAutoDeleteRowsFromEndRow:

    SecondsElapsed = GetTickCount - t

End Sub
share|improve this question
    
Whilst CR is the place to come for improving your code (including speed). It is not the place to seek explanation of why your code does what it does. This is the kind of question that belongs on Stack Overflow. – Zak Jan 12 at 12:01
1  
I suspect, however, that it's because the "delete operation" has some level of fixed overhead (I.E. re-formatting the sheet every time it's finished deleting) so calling it once (to delete 5 rows) has less overhead than calling it 5 times (to delete one row each time). – Zak Jan 12 at 12:05
    
Now that your question has mutated, some advice: Good CR questions are generally of the form: "My is designed to do XYZ, how could it have been done better?". Specifically, "Optimise" is a rather vague term. Optimise for what, specifically? – Zak Jan 12 at 12:13
    
Thanks @Zak, appreciate the feedback. Still trying to figure this site out, which has been frustrating at best – Jean-Pierre Oosthuizen Jan 12 at 12:15
1  
Also, this is a good place to start: Getting the best value out of Code Review - Asking Quesitons – Zak Jan 12 at 12:19
up vote 10 down vote accepted

Things I like:

  • Going last row to first, so as to avoid messiness with row numbers that occur when deleting whole rows.

  • Good use of With

  • Good function naming of GetTickCount (at least, assuming it does what it says it does)


And now that that's done:


Option Explicit

This should be at the top of every code module you ever create in VBA. Go to Tools --> Options --> Require Variable Declaration and it will automatically insert it for you from now on.

This is important because without it, VBA will interpret any new variable names (including mis-spellings) as entirely new variables, instead of what you intended.

It also forces you to declare your variables. So you must explicitly give them a type (Long, String, Variant etc.) and a scope (Procedure Dim var As, Module Private Var As, Project Public Var As).

This will then automatically catch all sorts of unintended situations (such as accidentally setting a number equal to an object) which would not be caught if VBA has to assume that all your variables are Variant because you never explicitly declared them.


Naming

Good Variable naming is one of the most important skills you can develop as a developer. Variable names (including values, objects, functions, even whole projects) should be Clear, Concise and, above all, Unambiguous.

Your variable names here are good. Your function name is not. Longer bears absolutely no relation to "Deleting Rows".

A function name like RemoveRowsWithEmptyCellsInRange would be far better.


Re-usability

Rather than hard-coding this function for column C of the active sheet, why not create a function to do it for any column at all?

Perhaps something like this?

Private Sub DescriptiveNameHere()

    Dim columnIndex As Long
    columnIndex = 3 '/ Column "C"

    Dim firstRow As Long, lastRow As Long
    firstRow = 20
    lastRow = Cells(Rows.Count, columnIndex).End(xlUp).row

    RemoveRowsWithEmptyCellsInColumn ActiveSheet, columnIndex, firstRow, lastRow

End Sub

Public Sub RemoveRowsWithEmptyCellsInColumn(ByRef targetSheet as Worksheet, ByVal columnIndex As Long, ByVal firstRow As Long, ByVal lastRow As Long)

    Dim ws as Worksheet
    Set ws = targetSheet
    ws.Activate

    Dim row As Long, col As Long
    Dim checkCell As Range

    col = columnIndex
    For row = lastRow To firstRow Step -1
        Set checkCell = ws.Cells(row, col)
        If IsEmpty(checkCell) Then Rows(row).Delete
        '/ You might also want to check for cells that appear empty but contain E.G. a formula that is currently displaying no value, like so:
        ' If checkCell.Text = "" Then Rows(row).Delete
    Next row

End Sub

Speed

Very low-hanging standard VBA fruit here:

Public Sub DisableApplicationSettings()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlCalculationManual

End Sub

Public Sub ResetApplicationSettings()

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlCalculationAutomatic

End Sub

Put that at the start and end of any VBA program and it will run inordinantly faster.

However, it doesn't take into account how the spreadsheet was before your program started (what if your user already had calculation set to manual?).

So, these, coupled with some public variables and some error handling so premature ending of the code execution still restores the settings, is even better:

Public VarScreenUpdating    As Boolean
Public VarEnableEvents      As Boolean
Public VarCalculation       As XlCalculation

Public Sub StoreApplicationSettings()

    VarScreenUpdating = Application.ScreenUpdating
    VarEnableEvents = Application.EnableEvents
    VarCalculation = Application.Calculation

End Sub

Public Sub DisableApplicationSettings()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlCalculationManual

End Sub

Public Sub RestoreApplicationSettings()

    Application.ScreenUpdating = VarScreenUpdating
    Application.EnableEvents = VarEnableEvents
    Application.Calculation = VarCalculation

End Sub
share|improve this answer

This code defines a few constants, and uses a With block, but otherwise achieves the result in a single line.

The code would be easier to read and maintain if I broke the line down into 2-3 lines, but I couldn't resist the brevity.

I haven't bothered persisting and restoring the ScreenUpdating, Calculation and EnableEvents properties, because I'm only performing a single operation.

Sub DeleteRows()

  Const ColName As String = "C"
  Const FirstRow As Long = 20

  With ActiveSheet
    .Range(ColName & FirstRow & ":" & ColName & .Range(ColName & .Rows.Count).End(xlUp).Row).SpecialCells(xlCellTypeBlanks).EntireRow.Delete
  End With

End Sub
share|improve this answer
    
Thanks @ThunderFrame, you answer works perfectly. Time 16ms without disabling the ScreenUpdating. With disabling time is 0ms. – Jean-Pierre Oosthuizen Jan 15 at 7:25
    
0ms is impressive, but it does take time... It's just that GetTickCount isn't very high resolution. If you want better timing, have a look at QueryPerfomanceCounter and QueryPerfomanceFrequency functions – ThunderFrame Jan 15 at 8:27
    
I should have thought of this, nice work :) – puzzlepiece87 Jan 21 at 17:22

The code below uses 7 lines verse 17.

It deletes the Rows individually as opposed to deleting a Range of blanks cells.

Unfortunately, it does take a little longer than the original code as posted in the question. So, it will only be better in terms of reducing code length.

Sub Longer()
'140ms to remove blanks rows of 80 rows

'Searches the designated Column and checks for a Blank Cell
'If there is a blank cell it deletes the row
    t = GetTickCount
    With ActiveSheet

       Firstrow = 20
       LastRow = .UsedRange.Rows(.UsedRange.Rows.Count).Row - 1

       For i = LastRow To Firstrow Step -1

           If .Cells(i, "C").Value = Empty Then .Cells(i, "C").EntireRow.Delete

       Next i

    End With

    timetaken = GetTickCount - t

End Sub

Instead of starting another Loop to find the Range of blank cells, rather just delete that row and move onto the next row. This simplifies the reading of the code in terms of understanding the Ranges and the additional Variables.

This code also removes the sort of Error Handling used to cater for the performing the Blank Range check when the For i…. gets to the FirstRow

share|improve this answer
    
Comments are not the place for extended discussion. Can we move this to chat if its going to go on? We have both the CR Help Desk and 2nd monitor to help discuss improvements to a review at length. – Dan Pantry Jan 12 at 13:28
    
Under normal circumstances, I would consider this a just-about-OK answer and probably not vote on it, but I like to reward people willing to self-improve so here, have an upvote ^^ – Zak Jan 12 at 13:30
    
Using UsedRange won't work if the used range doesn't start on the first row. – ThunderFrame Jan 13 at 9:15
    
@ThunderFrame what would the best option be given that my data set doesn't start in the first row or cell? – Jean-Pierre Oosthuizen Jan 13 at 9:16
    
My answer handles that, and is probably the fastest method here. – ThunderFrame Jan 14 at 19:10

Taking Things Further:

One thing that surprised me about both Zak and Jean-Pierre's answers is that neither maximized their range deletion reasoning. Both deleted using (potentially) several stepwise ranges. Here's how I do one deletion no matter what pattern of blanks occurs:

Option Explicit

Sub M1DeleteRowsWhereColCBlank()
'
' M1DeleteRowsWhereColCBlank Macro
'
' Keyboard Shortcut: Ctrl+Shift+Q
'

Dim rngAllData As Range
Dim rngBlankColC As Range
Dim Cell1 As Range
Dim lngLastRow As Long

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual

    With ActiveSheet
        lngLastRow = .Range("A" & .Rows.Count).End(xlUp).Row
        Set rngAllData = .Range(.Cells(20, 3), .Cells(lngLastRow, 3))

        For Each Cell1 In rngAllData
            If Cell1.Value = "" Then
                If rngBlankColC Is Nothing Then
                    Set rngBlankColC = Cell1
                Else
                    Set rngBlankColC = Union(rngBlankColC, Cell1)
                End If
            End If
        Next Cell1

        If Not (rngBlankColC Is Nothing) Then rngBlankColC.EntireRow.Delete

    End With

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic

End Sub

Some Code Notes:

  • I am more flexible about best practices than Zak. If you had to learn from someone, I recommend learning from him. I have outlined our differences below:
    • Zak's points concerning the user's existing preferences and reusability are great, but I will not include them in my answer.
    • One common convention that Zak does not use that I really like is including variable type prefixes. You may find that useful too.
    • I agree with Zak about using descriptive procedure names and variable names. I prefix my macros with M1, M2, etc. to order them in GUI macros tool. I don't name a variable descriptively or with camelCase when I am going to reuse it for many or indiscriminate purposes (like Cell1).
    • You can disable events like Zak does but I do not find the speed advantages to be worth bothering.
    • Zak uses a private and public sub. It's a good practice for parameterization but I don't include them in my answer.
    • As Zak alludes to and I find important enough to include in uncommented code, Excel sometimes cannot detect a cell as empty even when it has nothing in it due to formatting errors or formulas. I actively use Cell.Value = "" to prevent this.
  • As you can find discussed online, I do not find UsedRange to be as reliable as I need; .End(xlUp) has some advantages.
  • This code was tested on Excel 2010.

I understand that you prefer shorter code, so this may not fit your needs, but I think there are enough potential speed and error prevention improvements in here that future readers may find this answer worthwhile.

share|improve this answer
    
@zak, thank you for teaching such good practices to people seeking review. I strongly prefer flexibility between good practices and timesaving for myself but I am glad that people can learn from the way you are doing it. – puzzlepiece87 Jan 12 at 16:54
    
Thanks for the answer. Definitely some gems in there for future snd present use – Jean-Pierre Oosthuizen Jan 12 at 17:03
    
@puzzlepiece87 thanks. Just one thing, I don't use camelCase for my variables? that's news to me! – Zak Jan 12 at 17:39
    
@Zak I am obviously blind :P I must have temporarily been looking at Jean-Pierre's answer when scrolling back and forth. Fixing! – puzzlepiece87 Jan 12 at 17:41
1  
Also, didn't know you could use Union().entirerow like that, good to know. – Zak Jan 12 at 17:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.