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
|