Is This Good?

[DS]

I have this code that works, but as always, can it be better?
Any suggestion or help is apprecited.
Thanks
DS


Dim PO As Integer
Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = "
& Me.TxtCheckID & ""), 0)

Me.TxtName = DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber & "")

For PO = 0 To Nz(Me.TxtNumber - 1, 0)
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber & ""), "")
If IsNull(Me.TxtName) Or _
Me.TxtName = "" Then
Me.TxtName = ""
Else:
Set Application.Printer = Application.Printers(Me.TxtName.Value)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " &
Me.TxtCheckID &" & _
"And PDPrinterID = " & Me.TxtNumber & ""
Me.TxtNumber = Me.TxtNumber - 1
Me.TxtEmpty = Nz(DCount("PDPrinterID", "tblPrintDetails",
"PDPrinterID = " & Me.TxtNumber & " AND PDCheckID = " & Me.TxtCheckID &
""), 0)
If IsNull(Me.TxtEmpty) Or _
Me.TxtEmpty = "" Or _
Me.TxtEmpty = 0 Then
Me.TxtNumber = Me.TxtNumber - 1
Else:
End If
Set Application.Printer = Nothing
End If
Next PO

 

[John Vinson]

I have this code that works, but as always, can it be better?
Any suggestion or help is apprecited.
Thanks
DS

Well, it's not at all clear to me what you're trying to do, but you
have a lot of belts, suspenders, braces, and galluses - redundant
code!

Dim PO As Integer
Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = "
& Me.TxtCheckID & ""), 0)

Appending an empty string with & "" does ABSOLUTELY NOTHING. You can
remove all of these.

Me.TxtName = DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber & "")

Any reason NOT to just bind txtName and txtNumber to fields in
tblPrinters, using a Query joining your form's table to tblPrinters by
PDCheckID? Then you wouldn't need any lookups at all.

For PO = 0 To Nz(Me.TxtNumber - 1, 0)
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber & ""), "")

Rather than looping on a Form Control, I'd just copy the value in
txtNumber into an integer variable and loop on that - or, even better,
opening a Recordset on tblPrinters to find the names, and just loop
the recordset.

If IsNull(Me.TxtName) Or _
Me.TxtName = "" Then
Me.TxtName = ""

Since txtName can never be NULL - you set it using NZ; and since what
you're saying here is if it's equal to "" then set it to "", this
statement does nothing at all.

Else:
Set Application.Printer = Application.Printers(Me.TxtName.Value)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " &
Me.TxtCheckID &" & _
"And PDPrinterID = " & Me.TxtNumber & ""
Me.TxtNumber = Me.TxtNumber - 1
Me.TxtEmpty = Nz(DCount("PDPrinterID", "tblPrintDetails",
"PDPrinterID = " & Me.TxtNumber & " AND PDCheckID = " & Me.TxtCheckID &
""), 0)

I have no idea what this is intended to accomplish. However, txtEmpty
will get overwritten each time through the loop.

If IsNull(Me.TxtEmpty) Or _
Me.TxtEmpty = "" Or _
Me.TxtEmpty = 0 Then

Again, you set txtEmpty using NZ so it's either a number (a DCount) or
it's zero. It can never be NULL and it can never be an empty string.

Me.TxtNumber = Me.TxtNumber - 1
Else:
End If
Set Application.Printer = Nothing
End If
Next PO

Yep... could use some work!
Care to explain what you're trying to ACCOMPLISH?

John W. Vinson[MVP]

 

[DS]

Well, it's not at all clear to me what you're trying to do, but you
have a lot of belts, suspenders, braces, and galluses - redundant
code!



Appending an empty string with & "" does ABSOLUTELY NOTHING. You can
remove all of these.




Any reason NOT to just bind txtName and txtNumber to fields in
tblPrinters, using a Query joining your form's table to tblPrinters by
PDCheckID? Then you wouldn't need any lookups at all.




Rather than looping on a Form Control, I'd just copy the value in
txtNumber into an integer variable and loop on that - or, even better,
opening a Recordset on tblPrinters to find the names, and just loop
the recordset.




Since txtName can never be NULL - you set it using NZ; and since what
you're saying here is if it's equal to "" then set it to "", this
statement does nothing at all.




I have no idea what this is intended to accomplish. However, txtEmpty
will get overwritten each time through the loop.




Again, you set txtEmpty using NZ so it's either a number (a DCount) or
it's zero. It can never be NULL and it can never be an empty string.




Yep... could use some work!
Care to explain what you're trying to ACCOMPLISH?

John W. Vinson[MVP]

Thank you John,,,OK Here it is in a nutshell...
I'm looping through Printers,Starting with the highest and stopping at 0
because a Check can have multiple printers to print to.

I have a table called tblPrintDetails
It contains the CheckID, PrinterID, ItemID...

1) I get the highest Printer Number with the same CheckID from the
tblPrintDetails table.

2) This tells me how may times I need to loop thrugh to get to zero,
(One problem is that lets say I get 7 forthe highest numnber I might not
have 7 printers though and thats why I added the Me.TxtName="" because
it would cause an error.

3)Base on the PrinterNumber I lookup the Printer Name because the Set
Application.Printer doesn't seem to accept a Number.

4) I then print to that printer all of the ItemID's that match the
Printer and Check

5) I then lower the number by one and repeat untill the Printer Number is 0.

Like I said the problem lies where there is no consecutive printer. The
ideas you presented sound great, but perhaps a little beyond my skill
level at theis point, so as usual any help would be appreciated.
Thank you
DS

 

[DS]

DS wrote:
PS, I like how the Recordset Sounds, How would I do that? The query
added to the form might not be good as I already have a ton of stuff
going on the form already...Once again, Thank You
DS

 

[DS]

Hi John,
I removed some lines and syntax on your suggestions, it's cleaner and
this is as about as far as I could get without getting an error. The
main error kept coming when there was no printer to assign to
Application.Printers(Me.TxtName.Value)
so thus the redundant Me.TxtName = ""
I also did the TxtEmpty as a Dim statement...Dim EMPTI as Integer
What I would like to do is that recordset thing but alas as explained
earlier, beyond me at this point. Here is what I have at this point...

It works with either the EMPTI statement or not...???? Go figure!
Thanks
DS

'JOHN VINSON
Dim PO As Integer
Dim EMPTI As Integer
Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = "
& Me.TxtCheckID), 0)

For PO = 0 To Nz(Me.TxtNumber - 1, 0)
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber), "")
If IsNull(Me.TxtName) Or _
Me.TxtName = "" Then
Me.TxtName = ""
Else:
Set Application.Printer = Application.Printers(Me.TxtName.Value)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " &
Me.TxtCheckID & " " & _
"And PDPrinterID = " & Me.TxtNumber & ""
Me.TxtNumber = Me.TxtNumber - 1

'OPTIONAL*******************************************************
EMPTI = Nz(DCount("PDPrinterID", "tblPrintDetails", "PDPrinterID = " &
Me.TxtNumber & " " & _
"AND PDCheckID = " & Me.TxtCheckID), 0)
If IsNull(EMPTI) Or _
EMPTI = 0 Then
Me.TxtNumber = Me.TxtNumber - 1
Else:
End If
'OPTIONAL END*****************************************************
Set Application.Printer = Nothing
End If
Next PO

 

[John Vinson]

Like I said the problem lies where there is no consecutive printer. The
ideas you presented sound great, but perhaps a little beyond my skill
level at theis point, so as usual any help would be appreciated.

You may want to post a new thread. I've got a rush job to finish and
will be taking a week off starting Thursday, and this looks like a
pretty big chore - not difficult, but more time than I have right now!
Sorry!

John W. Vinson[MVP]

 

[DS]

You may want to post a new thread. I've got a rush job to finish and
will be taking a week off starting Thursday, and this looks like a
pretty big chore - not difficult, but more time than I have right now!
Sorry!

John W. Vinson[MVP]

Thank you John, for your honesty.
DS

 

[DS]

PMFJI, but you have a (IMO) programing error in your code!

You should *never* (I was taught) change a FOR...Next loop start or end
values inside the code:

Dim PO As Integer
Dim EMPTI As Integer

Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = " &
Me.TxtCheckID), 0)

For PO = 0 To Nz(Me.TxtNumber - 1, 0)
'
' some statements
'
Me.TxtNumber = Me.TxtNumber - 1

'
'more statements

Next PO


If you want to change the control on the form to display which printer is
printing, use


Me.TxtNumber = PO



Also, the proper way to cound down in a For...Next loop is:

Dim x as Integer

x = Me.TxtNumber

For PO = x To 0 Step -1
' statments

Next PO


Look at this modified code - (this is untested)
'-------------------------------------------------------------
Dim PO As Integer
Dim PO_Start As Integer
Dim EMPTI As Integer

Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = " &
Me.TxtCheckID), 0)
PO_Start = Me.TxtNumber

For PO = PO_Start To 0 Step -1
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
PO), "")
If Not Me.TxtName = "" Then
Me.TxtNumber = PO
Set Application.Printer = Application.Printers(Me.TxtName)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " & Me.TxtCheckID
& " " & _
"And PDPrinterID = " & PO & ""

Set Application.Printer = Nothing
End If
Next PO
'-------------------------------------------------------------



If you *have* to use

Me.TxtNumber = Me.TxtNumber - 1

maybe you should be using a DO...LOOP

(untested code)
-------------------------------------------------------------
Dim PO As Integer
Dim EMPTI As Integer

'note the -1 at the end of the following statement
Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = " &
Me.TxtCheckID), 0) - 1

Do
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber), "")
If Not Me.TxtName = "" Then
Set Application.Printer = Application.Printers(Me.TxtName.Value)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " & Me.TxtCheckID
& " " & _
"And PDPrinterID = " &
Me.TxtNumber & ""

Me.TxtNumber = Me.TxtNumber - 1

Set Application.Printer = Nothing
End If
Loop Until Me.TxtNumber = 0

Wow!!! Thank you for your advice, I will get to it immediatly and test
it out. I appreciate it.
Thanks
DS

 

[DS]

Wow!!! Thank you for your advice, I will get to it immediatly and test
it out. I appreciate it.
Thanks
DS

Thanks Steve, Just got done testing them, I tried the first one and
received no response. The second one works except in the following
circumstances....If their are no records to print it locks up the
program, the other instance is if there is only One printer and it's
printer number one, nothng happens. How would I code this to stop the
code from running if there are no records to print? Also, maybe an if
statement if the first printer is 1. I'm a little scared at this
point, what I wrote seems to be working, but you seem to have more
knowledge than I do and I'm a little worried that I'll get bit somewhere
down the road with my "seat of the pants" code! Any help is greatly
appreciated.
Thanks
DS

 

[DS]

This works in all circumstances!!!!

Thanks to all that helped.
DS


Dim PO As Integer
Dim EMPTI As Integer
Me.TxtNumber = Nz(DMax("PDPrinterID", "tblPrintDetails", "PDCheckID = "
& Me.TxtCheckID), 0)
On Error GoTo ErrorHandler
Do
Me.TxtName = Nz(DLookup("PrinterName", "tblPrinters", "PrinterID = " &
Me.TxtNumber), "")
If Not Me.TxtName = "" Then
Set Application.Printer =
Application.Printers(Me.TxtName.Value)
DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " &
Me.TxtCheckID & " And PDPrinterID = " & Me.TxtNumber & ""
Me.TxtNumber = Me.TxtNumber - 1
'<<<
EMPTI = Nz(DCount("PDPrinterID", "tblPrintDetails",
"PDPrinterID = " & Me.TxtNumber & " " & _
"AND PDCheckID = " & Me.TxtCheckID), 0)
If IsNull(EMPTI) Or _
EMPTI = 0 Then
End If
'<<<
Set Application.Printer = Nothing
End If
Loop Until Me.TxtNumber = 0
'Send Items
DoCmd.SetWarnings False
SENDSQL = "UPDATE tblCheckDetails SET [CDSent] = True " & _
"WHERE tblCheckDetails.[CDCheckID] = Forms!Form3![TxtCheckID]; "
DoCmd.RunSQL (SENDSQL)
DoCmd.SetWarnings True
End
ErrorHandler:
DoCmd.OpenForm "frmMsgWarning"
Forms!frmMsgWarning!TxtMsg = "INVALID PRINTER"
End Sub

 

[DS]

DS,

Comments in line.......

Application.Printers(Me.TxtName.Value)

Value is the default property for the control; it is not necessary to add
".Value".

DoCmd.OpenReport "rptPrepSlip", , , "PDCheckID = " &
Me.TxtCheckID & " And PDPrinterID = " & Me.TxtNumber & ""

Why do you add &"" ??? If Me.TxtNumber is a number, it doesn't need a
delimiter. While I'm asking, is it text or numeric? And what is it? The name
is not very descriptive. You might be the only one to see the code, but is 6
months will you look at the code and think "WHAT was I thinking?? " ?(BTDT)
Maybe PrtNumber (printer number) would be a more descriptive name for the
control.

'<<<
EMPTI = Nz(DCount("PDPrinterID", "tblPrintDetails",
"PDPrinterID = " & Me.TxtNumber & " " & _
"AND PDCheckID = " & Me.TxtCheckID), 0)
If IsNull(EMPTI) Or EMPTI = 0 Then

End If
'<<<

(I modified this a little for clarity.) I don't understand why you have this
in the code. It doesn't do anything!!

Loop Until Me.TxtNumber = 0

This might cause an error if there is not a printer number 0. You might try

Loop Until Me.TxtNumber < 1

DoCmd.SetWarnings False
SENDSQL = "UPDATE tblCheckDetails SET [CDSent] = True " & _
"WHERE tblCheckDetails.[CDCheckID] = Forms!Form3![TxtCheckID]; "
DoCmd.RunSQL (SENDSQL)
DoCmd.SetWarnings True

Two things here:

1) Why are you using "Forms!Form3![TxtCheckID]" when you used
"Me.TxtCheckID" earlier in the code?

2) Many of the MVPs recommend using

CurrentDB.Execute SENDSQL, dbFailOnError

If there was an error, with "DoCmd.SetWarnings False", you would not be
notified.

End

Instead of End to exit the Sub, use the Exit Sub command.

ErrorHandler:
DoCmd.OpenForm "frmMsgWarning"
Forms!frmMsgWarning!TxtMsg = "INVALID PRINTER"

Why open a form? Why not use

MsgBox "INVALID PRINTER"



These are my thoughts, observations and how I would to things and not meant
to be insulting or a critique of your programming methods.

I've seen many of your previous posts and you seem to progressing a a good
pace. When I started learning how to program, I wish I had a forum like this
to help me.

HTH

Thanks Steve, I appreciate the feedback! No insult taken! I appreciate
the input, And your right, I find this forum to be a valuable means of
learning to program. Once again Thank you very much, I will apply your
recommendations.


The EMPTI checks for records in the tblPrintDetails Table. I found that
without it I would get an error message when there were no records to print.

I didn't know &"" was, I've been kind of using it all along. So this
is for Text only?

Once again,
Many Thanks
DS

 

[DS]

If you comment out the code between the " '<<< " and manually step
thru the code one line at a time, do you still get the error. If not, it
sounds like a timing error; the code is trying to print to the next printer
before the first report is printed.

Steve,how do you step through code?

As to the code, you set EMPTI equal to DCOUNT(), then you do nothing with it;
the IF() statement doesn't have any commands in it!

Well, if its empty I want to blow by it and go to the next printer.
It seems to work....but I'm open to suggestions!

Thanks
DS

 

[DS]

Steve,how do you step through code?


Well, if its empty I want to blow by it and go to the next printer. It
seems to work....but I'm open to suggestions!

Thanks
DS

PS EMPTI has this line if EMPTI = 0 then
Me.TxtNumber = Me.TxtNumber - 1
else:
end if

 

[DS]

:




It looks like you added

Me.TxtNumber = Me.TxtNumber - 1

in the IF() statement. Now there is a statement to execute.

But why did you add "Else"? Read help on IF().

You have a condition (EMPTI = 0). If the condition is TRUE, then you execute
one or more statements; in your case the statement is "Me.TxtNumber =
Me.TxtNumber - 1". If the condition is FALSE, you execute the statements
after the ELSE. The "ELSE" is not required if there are no statements to
execute if the condition is FALSE.
So you can just use

If EMPTI = 0 Then
Me.TxtNumber = Me.TxtNumber - 1
End If


And you shouldn't have a colon after the ELSE. Can I ask why you added it?

Thanks, Why did I add the colon? To be honest I don't know! But it's
great to know that I can just do End If.
tblPrinters holds the name of the rinters on that system. These are
applied to menu items. PrintDetails contains the information after an
item is ordered, it say which printer it goes to.
You've been very helpful and informative! Once again I'd like to thank you!
DS