First, thank you very much for improving your posting style -- it really helps.
I'm looking at your steps, and trying to relate them to the long piece of code you posted -- it's not easy, because the terms (variable names, table column names, etc) that appear in the code do not match the terms you use in describing your two steps.
Apart from that, there is a lot that is wrong with your code as posted. For example, look at how $i is used -- set to zero at the top, then used as a loop counter when cycling through data from a query, then used in while ($i<=2) (it gets incremented inside the loop, but it's probably already greater than 2 before you reach the while loop). Other problems:
- You do two separate database connections -- are these two separate databases? If not, you should connect just once.
- Inside the while loop, you have "my $rd", and you declare a large subroutine that depends on having $rd in scope -- it would be better to pass $rd as a parameter to the sub, so the sub does not need to be inside the while loop.
- There's not nearly enough error checking.
Rather than go further into the problems, let's see if I can rephrase the steps and the code -- you can tell me if I'm guessing correctly about what the task really is:
- Get a list of ids for data recipients from the database -- each id will be used to query for the data that needs to be sent, and as part of the email address to send to.
- For each id in the list:
- query the database to get the data for that recipient
- create an excel file and load it with this person's data
- send email to the recipient (and attach the excel file?)
I think this is what the code ought to look like -- you'll need check all the details carefully, and make corrections if I've misunderstood anything. Obviously, I can't really test it, but it passes "perl -cw" with no problems.
Just a few closing remarks about my version vs. yours:
- Note that the "main" part of the program is quite brief, and someone reading the code can see pretty easily what it does, and how it relates to the description I gave above -- in fact, the description and the main part of the code are almost the same thing.
- Since I removed all the five-line comments from your script (and many of the blank lines around them), more of the actual code is visible on the screen at one time, making it easier to see how variables are used, how the logic flows, etc. Most of your commenting was unnecessary -- it took up a lot of space just to say things that should be obvious from the code itself.
- I've tried to keep most of the variable declarations close to where they are used, also making it easier to see the strategy and flow of the code.
- I've made the subroutines independent of the main part of the code, passing the parameters that are needed for each sub, making the subs more modular and self-contained (helpful when you want to reuse code, which usually involves moving subs into separate modules/packages).
- In the part that sets up the excel file, I've used a hash or array together with a loop instead of repeating a bunch of subroutine calls -- the less repetitive typing you do when coding, the better.
Again, I'm not sure this really does what you want, but I hope it's moving you in the right direction. | [reply] [d/l] [select] |