[EMAIL PROTECTED] ([EMAIL PROTECTED]) wrote on 15 Nov 2001:
> i am having a problem reading from an array. i wrote this script to
> query a database, get a certain record and based on the number of
> records returned write to a file 1.the unixtime, 2.the number of
> records in asteriks (ex. *** for 3 records) and 3.the number of
> records in digit form.
>
> i have the script querying every hour for the last 2 hours and the
> feeding the hour, asteriks, and number of records into an array
> (@result) and then i want to print each $result[1..#] into an html
> file. for reasons unclear to me i receive the exact same hour record
> ($starthour) for each returned query and then only one asterik for the
> first record and two for the second for the number of records returned
> and then that number in digit form
> (ex: 1005802873 * 1
> 1005802873 ** 2 )
> but this result inaccurate, if i only query one hour back and dont
> save the results in @result i receive an accurate result. can someone
> point me in the right direction as to why this is happening. please
> note this is my first real perl script and is probably hard to read,
> any suggestions on how to improve readability will be greatly
> appreciated.
You're not updating your $mailin variable anywhere, so the value of $time
when $mailin is initialized is the one always used.
As far as style goes,
perldoc perlstyle
has many suggestions for writing readable code. You might also take a
look at perltidy: http://perltidy.sourceforge.net/
I noticed lots of places where you said something like
$starthour = ($time - 3600);
The parentheses aren't really necessary in this case. You might consider
whether or not the code looks cleaner if you simply say
$starthour = $time - 3600;
There are times when parens are required (lists, etc) or when you want to
make the order of operation clear, but in the above case I think they're
just clutter. YMMV.
>
> thanks alot,
> josh
>==========================below is the script========================
> #/usr/bin/perl -w
> use strict;
> use DBI;
> use Time::Local;
>
> my ($database, $user, $password, $hostname, $dbh, $sth, $starthour,
> $rc); my ($numrows, $final, $item, $mailin, $endday, $time);
> my (@result, @info);
Declaring everything at the beginning means all your variables are global
variables. Just declare them as you need them, and confine them to the
smallest possible scope; that will make debugging easier as you write
longer programs.
perldoc perlsub # "Private Variables via my()"
http://perl.plover.com/FAQs/Namespaces.html # Coping with Scoping
> $database = "sendmail";
> $user = "josh";
> $password = "buzaglo";
> $hostname = 'katan';
>
>
> #this gets time and converts it to unixtime
> my
> ($time,$dateout,$day,$sec,$min,$hour,$mday,$mon,$year,$wday,$yday,
> $isds
> t,$dst);
> ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) = localtime(time);
> $time = timelocal($sec,$min,$hour,$mday,$mon,$year);
You don't need all this, or Time::Local. Just say
my $time = time();
>From perlfunc:
time
Returns the number of non-leap seconds since whatever time the system
considers to be the epoch (that's 00:00:00, January 1, 1904 for MacOS,
and 00:00:00 UTC, January 1, 1970 for most other systems).
Getting picky: you might consider calling it something like $time_now.
It sounds trivial, but if you come back to a program years later, you want
the variable names to be as obvious as you can make them.
[Oh. I just noticed that you're using $mday and $mon at the bottom of the
program. See comments there.]
> $starthour = ($time - 3600);
> $endday = ($time - 7200);
>
> # connect to db
> if (not $dbh = DBI-> connect
> ("dbi:mysql:database=$database;host=localhost",
> "$user", "$password"))
> {
> print "unable to connect to $database $!";
> }
"By default, DBI->connect sets PrintError ``on''", so this shouldn't
really be necessary.
However, if you want the program to die if it encounters an error, use
something like this
$dbh = DBI->connect(
"dbi:mysql:database=$database;host=localhost",
"$user", "$password", {RaiseError => 1}
)
>
> sub select
Perl already has a built-in function named 'select'. You might consider
renaming this to something else. Another person might read your code and
wonder why you're redefining select().
You could move your subroutine definition to the end of the program (or
the beginning) instead of sticking it in the middle of everything. It
doesn't really matter as far as perl is concerned, but it might make the
program easier for humans to read. I prefer putting them at the end; you
get straight to the action, and can look up what the subs do as you go.
> {
> my ($query) = @_;
This works ok as things are, but
my $query = shift;
is a bit cleaner and more standard. It also only pulls off *only* the
first scalar from @_
> $sth = $dbh->prepare($query);
> if (not $sth)
> {
> print "problem with statement handle $!\n";
> }
> $rc = $sth->execute();
> if (not $rc)
> {
> print "problem executing $!\n";
> }
> $numrows == 0;
>
> while ( my @row = $sth->fetchrow_array ) {
> foreach $item (@row) {
> $numrows++;
> $final = ("*" x $numrows);
> push(@result, $starthour, $final, $numrows);
> }
> }
If all you're doing is counting the number of rows, you don't need to loop
over each of the fields within a row.
while ( my @row = $sth->fetchrow_array ) {
$numrows++;
}
$final = ( "*" x $numrows );
push ( @result, $starthour, $final, $numrows );
> }
>
>
> $mailin = ("SELECT mto from log where mto like '%kata%' and datein
> between $starthour and $time;");
Move this statement to the beginning of the while() loop below.
Otherwise, you're just using the same statement over and over, with the
results you noted.
>
> while ($time > $endday)
> {
my $mailin = "SELECT mto from log where mto like '%kata%' and datein
between $starthour and $time;";
> &select($mailin);
The & isn't really necessary; see perlsub.
> $time = ($time - 3600);
> }
>
>
> #print results
> my $present = ($year + 1900);
> open(DAILY,">>$mday-$mon-$present-mailin.html") || die "can't open
> daily.html $!\n";
my ($day, $month, $year) = (localtime())[3,4,5];
$year += 1900;
open(DAILY,">>$day-$month-$year-mailin.html")
|| die "can't open file for output: $!\n";
This way you don't need all those variables from the beginning of the
program. Everything is right here. (Of course, you're free to disagree
-- you may want all those variables for other reasons.)
>
> # Content-type: text/html
> print DAILY <<EOL;
>
><HTML>
> <HEAD>
> <TITLE>Daily incoming mail stats by hour</TITLE> </HEAD>
> <BODY>
> <H1>hour number of incoming emails</H1>
> $result[0] $result[1] $result[2]
> $result[3] $result[4] $result[5]
></BODY>
></HTML>
>
> EOL
How is this going to look when you run the program multiple times in one
day?
Hope this helps.
--
David Wall
[EMAIL PROTECTED]
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
