Please take a look at this working code and critque it.

[BerkshireGuy]

This code is working, but I have the feeling its not the best code.

Basically, I have a sheet in which if column D is equal to zero, I want
to delete the row and shift the rows up. I want to do this for every
row, starting at row three to bypass the headings.

Please let me know your thoughts.

With objWkb.Worksheets("Sorted Rankings")
TotalRows = objWkb.Worksheets("Sorted Rankings").Range("D" &
objWkb.Worksheets("Sorted Rankings").Rows.Count).End(xlUp).Row

For Row = 3 To TotalRows Step 1
If Row > TotalRows Then GoTo EndNow:

If .Cells(Row, "D").Value = 0 Then
.Rows(Row).EntireRow.Delete ' Delete Shift:=xlUp
Row = Row - 1 ' Because we deleted a row and shifted it up, we have
to make sure variable Row does not get increased otherwise the next row
down from a deleted row will not be evaluated!
End If
'Check TotalRows in case a row was deleted
TotalRows = objWkb.Worksheets("Sorted Rankings").Range("D" &
objWkb.Worksheets("Sorted Rankings").Rows.Count).End(xlUp).Row

Next Row
EndNow:

 

[STEVE BELL]

When deleting rows - you need to work from the bottom up...

For Row = TotalRows to 3 Step -1

 

[JE McGimpsey]

Couple of comments:

1) It's nearly always a bad idea to use reserved words as variable names
(e.g., "Row"). It can get confusing, especially months down the road.

2) You can avoid all the messy row handling if you go backwards through
the rows:

For nRow = nTotalRows To 3 Step -1
If Cells(nRow, "D").Value = 0 Then _
Rows(nRow).EntireRow.Delete
Next nRow

3) You can gain a lot of efficiency if there are a large number of rows
to delete by identifying them all and deleting them all at once (e.g.,
XL doesn't have to reindex each time). Here's one way I might rewrite
your code:

Dim rCell As Range
Dim rDel As Range
With objWkb.Worksheets("Sorted Rankings")
For Each rCell In .Range("D3" & _
.Range("D" & .Rows.Count).End(xlUp).Row)
With rCell
If .Value = 0 Then
If rDel Is Nothing Then
Set rDel = .Cells
Else
Set rDel = Union(rDel, .Cells)
End If
End If
End With
Next rCell
If Not rDel Is Nothing Then rDel.EntireRow.Delete
End With

 

[keepITcool]

nitpicking... <g>

using a union can get slow if the union has many areas.. and
will get impossibly slow around 800 areas and more.

better to "flush" the range if the area count gets 'too high'.

Sub foo()

Dim rCell As Range
Dim rDel As Range
With objWkb.Worksheets("Sorted Rankings")
For Each rCell In .Range("D3" & _
.Range("D" & .Rows.Count).End(xlUp).Row)
With rCell
If .Value = 0 Then
If rDel Is Nothing Then
Set rDel = .Cells
Else
Set rDel = Union(rDel, .Cells)
If rDel.Areas.Count > 200 Then Call Flush(rDel)
End If
End If
End With
Next rCell
End With
Call Flush(rDel)
End Sub

Private Sub Flush(ByRef rDel As Range)
If Not rDel Is Nothing Then
Application.ScreenUpdating = False
rDel.EntireRow.Delete
Application.ScreenUpdating = True
Set rDel = Nothing
End If
End Sub





--
keepITcool
| www.XLsupport.com | keepITcool chello nl | amsterdam


JE McGimpsey wrote :

 

[BerkshireGuy]

Excellent! Thanks everyone.
-Brian

 

[BerkshireGuy]

KeepitCool -

Your code is generating a Method or data member not found error.

Thanks
Brian

 

[Dave Peterson]

And much more mundane...

If the cell is empty, then its value will be 0, too.

You may want to watch out for that, too.

 

[keepITcool]

possibly. it was untested and adapted from
JE McGimpsey, just to illustrate the flush
'technique'.

the objwkb is missing..
thats all, rest works for me.

include dave peterson's suggestion

With rCell
If Len(.Value) > 0 And .Value = 0 Then

and WHERE does it generate that error?


--
keepITcool
| www.XLsupport.com | keepITcool chello nl | amsterdam


BerkshireGuy wrote :