Reply
 
LinkBack Thread Tools Search this Thread Display Modes
  #1   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 47
Default The For Loop Worked, but need critique

It took me a day and a half, but I wrote my first "For
loop", and it does what I want it to. I am posting this to
invite critique to see how it could be more efficiently
written or cleaned up some if any.

The purpose of this loop was to work with Columns D and E,
which are page and line number columns respectively. They
are text entries when I get through with them even though
I am using a numeric value pagenumcounter and
linenumcounter to keep up with the values as they change.
But page 1 is shown in the field as 01, and line 1 is
shown as 01, so that they sort properly with page 10 and
line 10 values etc when they are in a database.

There are several constraints placed on the pagenum. Each
time, the range linenum gets to 12, then the next time the
loop processes the line, the pagenum is to go up by 1, and
the linenum is to go back to a 1, and start climbing
again. The other constraint is that if the value in column
B changes which is prmo, then we want to treat the page
and line number the same as if the line number gets to 12.
We want to start the line number back at 1 and add 1 to
the page number regardless of what the line number had
been. So, I compare two spreadsheet range
values "Currprmo" and "prevprmo" in the spreadsheet to
accomplish that part. I used spreadsheet range "currprmo"
to manage the current line being processed, and I used the
spreadsheet range "prevprmo" to look back at what the
value of prmo was in the previous line.

With all that said, here is my procedure for your critique:

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
For i = 1 To cRows
Range("Currprmo").Value = Range("B1").Offset(Range
("Rownow") - 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
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
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
Next
Range("d1").Select
End Sub




  #2   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 128
Default The For Loop Worked, but need critique

Hi

it's a long post but one thing that is always preferable is to address your
objects directly rather than by selecting them e.g.

untested, mind
Range("B2").copy
rather than
Range("B2").select
selection.copy

it is shorter to code and MUCH quicker to run

once your used to it, it is possibly easier to debug.

Regards

Tim


"Bruce Roberson" wrote in message
...
It took me a day and a half, but I wrote my first "For
loop", and it does what I want it to. I am posting this to
invite critique to see how it could be more efficiently
written or cleaned up some if any.

The purpose of this loop was to work with Columns D and E,
which are page and line number columns respectively. They
are text entries when I get through with them even though
I am using a numeric value pagenumcounter and
linenumcounter to keep up with the values as they change.
But page 1 is shown in the field as 01, and line 1 is
shown as 01, so that they sort properly with page 10 and
line 10 values etc when they are in a database.

There are several constraints placed on the pagenum. Each
time, the range linenum gets to 12, then the next time the
loop processes the line, the pagenum is to go up by 1, and
the linenum is to go back to a 1, and start climbing
again. The other constraint is that if the value in column
B changes which is prmo, then we want to treat the page
and line number the same as if the line number gets to 12.
We want to start the line number back at 1 and add 1 to
the page number regardless of what the line number had
been. So, I compare two spreadsheet range
values "Currprmo" and "prevprmo" in the spreadsheet to
accomplish that part. I used spreadsheet range "currprmo"
to manage the current line being processed, and I used the
spreadsheet range "prevprmo" to look back at what the
value of prmo was in the previous line.

With all that said, here is my procedure for your critique:

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
For i = 1 To cRows
Range("Currprmo").Value = Range("B1").Offset(Range
("Rownow") - 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
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
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
Next
Range("d1").Select
End Sub






  #3   Report Post  
Posted to microsoft.public.excel.programming
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


  #4   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 47
Default The For Loop Worked, but need critique

Bob:

What is the "CDO" that I am using that you are referring to?

Otherwise, I'm printing out your message so I can study it more
carefully.

Thanks,


Bruce



*** Sent via Developersdex http://www.developersdex.com ***
Don't just participate in USENET...get rewarded for it!
  #5   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 634
Default The For Loop Worked, but need critique

That's the horrible manky web based interface that you used to post your question with in the
first place. :-)

Seriously though, the best way to interact with these groups is through a newsreader such as
Outlook Express (And some will giv eyou just as much grief about that one as well, but it works
for me most of the time). Simply add a news service to it using the microsoft news server
msnews.microsoft.com and it is much easier to read/write/follow your messages than what you are
using.

That having beed said, when I log on from work, I don't have acces to news servers, they are
restricted, so the CDO interface is the only way I can access from work. At home though, it is a
much nicer experience all round.

Chip Pearson has some more info on his site at the following link:-

http://cpearson.com/excel/links.htm#Newsgroups

--
Regards
Ken....................... Microsoft MVP - Excel
Sys Spec - Win XP Pro / XL2K & XLXP

----------------------------------------------------------------------------
Attitude - A little thing that makes a BIG difference
----------------------------------------------------------------------------



"Bruce Roberson" wrote in message
...
Bob:

What is the "CDO" that I am using that you are referring to?

Otherwise, I'm printing out your message so I can study it more
carefully.

Thanks,


Bruce



*** Sent via Developersdex http://www.developersdex.com ***
Don't just participate in USENET...get rewarded for it!





  #6   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 620
Default The For Loop Worked, but need critique


That's the horrible manky web based interface that you used to post your

question with in the
first place. :-)


Hey Ken,

That was my question to be rude and superior about <vbg.

Bob


  #7   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 634
Default The For Loop Worked, but need critique

LOL - I didn't say half as many nasty things as I could have done, so I figured there was plenty
left for you. <vbg

--
Regards
Ken....................... Microsoft MVP - Excel
Sys Spec - Win XP Pro / XL2K & XLXP

----------------------------------------------------------------------------
Attitude - A little thing that makes a BIG difference
----------------------------------------------------------------------------



"Bob Phillips" wrote in message
...

That's the horrible manky web based interface that you used to post your

question with in the
first place. :-)


Hey Ken,

That was my question to be rude and superior about <vbg.

Bob




  #8   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 23
Default The For Loop Worked, but need critique

Blame the need on having to use the web based interface on the IT people at
my office. They get a power trip tieing accountants hands (especially mine
for some reason) <vbg

They have my Lotus Notes so tied down that I do well to send an email. And
they have the gall to lock me out of my own PC's access to the Outlook
Express directory.

So, I guess I'm stuck with this "manky web based approach" when at work. But
at home, its definitely Outlook Express for me.

Have a good weekend.

Bruce
"Bob Phillips" wrote in message
...

That's the horrible manky web based interface that you used to post your

question with in the
first place. :-)


Hey Ken,

That was my question to be rude and superior about <vbg.

Bob




  #9   Report Post  
Posted to microsoft.public.excel.programming
external usenet poster
 
Posts: 634
Default The For Loop Worked, but need critique

LOL - I know EXACTLY how you feel on that one.

--
Regards
Ken....................... Microsoft MVP - Excel
Sys Spec - Win XP Pro / XL2K & XLXP

----------------------------------------------------------------------------
Attitude - A little thing that makes a BIG difference
----------------------------------------------------------------------------



"Bruce Roberson" wrote in message
...
Blame the need on having to use the web based interface on the IT people at
my office. They get a power trip tieing accountants hands (especially mine
for some reason) <vbg

They have my Lotus Notes so tied down that I do well to send an email. And
they have the gall to lock me out of my own PC's access to the Outlook
Express directory.

So, I guess I'm stuck with this "manky web based approach" when at work. But
at home, its definitely Outlook Express for me.

Have a good weekend.

Bruce
"Bob Phillips" wrote in message
...

That's the horrible manky web based interface that you used to post your

question with in the
first place. :-)


Hey Ken,

That was my question to be rude and superior about <vbg.

Bob






Reply
Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes

Posting Rules

Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are On


Similar Threads
Thread Thread Starter Forum Replies Last Post
Find loop doesn't loop JSnow Excel Discussion (Misc queries) 2 June 24th 09 08:28 PM
The reply worked, but I still need help.... John1791 Excel Worksheet Functions 5 December 27th 07 04:57 PM
Most hours worked spankydata New Users to Excel 3 August 10th 06 09:51 AM
It Worked!!! :-) thelees Excel Discussion (Misc queries) 1 September 25th 05 11:22 PM
Hours worked Keith Bowman Excel Worksheet Functions 2 November 26th 04 07:07 PM


All times are GMT +1. The time now is 01:58 PM.

Powered by vBulletin® Copyright ©2000 - 2024, Jelsoft Enterprises Ltd.
Copyright ©2004-2024 ExcelBanter.
The comments are property of their posters.
 

About Us

"It's about Microsoft Excel"