View Single Post
  #3   Report Post  
Posted to microsoft.public.excel.programming
Bob Phillips[_5_] Bob Phillips[_5_] is offline
external usenet poster
 
Posts: 620
Default The For Loop Worked, but need critique

Bruce,

A few immediate thoughts. I know you use CDO, but I hope the lack of
indentation is caused by that, and that you use it. A few comments in the
code might be useful for later use.


Sub add_page_and_line()
Dim cRows As Long
cRows = Cells(Rows.Count, "A").End(xlUp).Row
Range("pagenumcounter").Value = 1
Range("linenumcounter").Value = 0
Range("rownow").Value = 1
Range("d1").Select


This last line seems superfluous, other cells are selected in the loop, and
I cannot see any action upon the selected cell

For i = 1 To cRows
Range("Currprmo").Value = Range("B1").Offset(Range ("Rownow") - 1,

0).Value

This is a personal thing, but I don't like defaulting properties. I would
always explicitly state what property I am using, in this case
Range("Currprmo").Value =
Range("B1").Offset(Range("Rownow").Value - 1, 0).Value

If i = 1 Then
Range("Prevprmo").Value = Range("B1").Value
Else
Range("Prevprmo").Value = Range("B1").Offset(Range("Rownow") -

2, 0).Value
End If
Range("rownow").Value = Range("rownow").Value + 1
If Range("Linenumcounter").Value + 1 = 13 Or

Range("Currprmo").Value _
< Range("Prevprmo") Then


Again, a personal thing, but I break each test to its own line,
If Range("Linenumcounter").Value + 1 = 13 Or _
Range("Currprmo").Value < Range("Prevprmo") Then
I find it more readable

Range("pagenumcounter").Value = Range("pagenumcounter").Value + 1
Range("linenumcounter").Value = 1
Else
Range("linenumcounter").Value = Range("linenumcounter").Value + 1
End If
ActiveCell.Value = "=pagenum"
Selection.Copy
Selection.PasteSpecial Paste:=xlValues, Operation:=xlNone,
SkipBlanks:= _
False, Transpose:=False
Application.CutCopyMode = False


All of these arguments are not necessary, the only non-default you are using
is Paste, so keep it simple
Selection.PasteSpecial Paste:=xlValues
and the same below

ActiveCell.Offset(0, 1).Select
ActiveCell.Value = "=linenum"
Selection.Copy
Selection.PasteSpecial Paste:=xlValues, Operation:=xlNone,
SkipBlanks:= _
False, Transpose:=False
Application.CutCopyMode = False
ActiveCell.Offset(1, -1).Select


This can all be simplified to, without doing the select. Select is slow and
costly (in processing terms), but much worse can be problemmatical, because
it means that the worksheet you are working on has to be visible. If it
isn't, you get the dreaded 1004 error (you've seen that haven't you), so it
should be avoided wherever possible
With ActiveCell.Offset(0, 1)
.Value = "=linenum"
.Copy
.PasteSpecial Paste:=xlValues
.Offset(1, -1).Select
End With
Application.CutCopyMode = False

Next
Range("d1").Select
End Sub


HTH

Bob