Beefy Boxes and Bandwidth Generously Provided by pair Networks
P is for Practical
 
PerlMonks  

Code Review

by PrimeLord (Pilgrim)
on Dec 12, 2002 at 22:01 UTC ( [id://219440]=perlquestion: print w/replies, xml ) Need Help??

PrimeLord has asked for the wisdom of the Perl Monks concerning the following question:

Hello Monks. I am hoping some of you can take a look at a program recently wrote and maybe make some suggestions on how it can be improved. The function of the program is to transfer copies of several reports from several sites each day. The program is run every 15 minutes out of a cron job between midnight and eight am.

Essentially it looks to see if the reports are completed at each site and if they are copies them over. I am using ssh commands to do the listings and copies. One of my concerns is that I am generating a ton of SSH connections and I would like to find a good way to cut that down. My Perl skills are improving, but are still a long way from being very good. So please be gentle. Any suggestions you might have would be greatly appreciated. Thank you. Below is a copy of the code.

use strict; use Time::Local; sub _init_values { my ($seconds, $minutes, $hours, $day, $month, $year) = localtim +e; my $date = sprintf("%04d%02d%02d", $year + 1900, $month + 1, $d +ay); my $report_dir = "/reports/directory"; my $config_dir = "/config/directory"; return $date, $report_dir, $config_dir, $hours; } sub _get_list { my ($date, $report_dir, $config_dir, $hours) = @_; my %reports; if ($hours == 00) { open IN, "$config_dir/report_master_list" or die "$!"; while (<IN>) { chomp; $reports{$_}++; } close IN or warn "$!"; _get_reports($date, $report_dir, $config_dir, \%reports) +; } elsif ($hours == 08) { open IN, "$config_dir/report_temp_list" or die "$!"; while (<IN>) { chomp; $reports{$_}++; } close IN or warn "$!"; open OUT, ">> /disk2/daily_report" or die "$!"; print OUT "\n\nDaily Reports That Were Unable To Be Tran +sferred\n"; print OUT "--------------------------------------------- +---\n\n"; for (sort keys %reports) { print OUT "$_\n"; } print OUT "\n\n"; unlink "$config_dir/report_temp_list" or warn "$!"; } else { open IN, "$config_dir/report_temp_list" or die "$!"; while (<IN>) { chomp; $reports{$_}++; } close IN or warn "$!"; _get_reports($date, $report_dir, $config_dir, \%reports) +; } } sub _get_reports { my ($date, $report_dir, $config_dir, $reports) = @_; my %site_map = qw( site1 fullsite1 site2 fullsite2 site3 fullsite3 site4 fullsite4 site5 fullsite5 site6 fullsite6 ); for (sort keys %$reports) { my ($report_name, $site) = split /\./; my @files = `ssh host.$site ls $report_dir`; for my $file (@files) { chomp $file; if ($file =~ /$report_name/ && $file =~ /$date/) +{ my $status = `ssh host.$site cat $report_d +ir/$file`; if ($status =~ /End of Report/) { my $transfer = `scp host.$site:$rep +ort_dir/$file /disk2/$site_map{$site}/Reports`; delete $reports->{$_}; } } } } open OUT, "> $config_dir/report_temp_list" or die "$!"; for (sort keys %$reports) { print OUT "$_\n"; } } my ($date, $report_dir, $config_dir, $hours) = _init_values; _get_list($date, $report_dir, $config_dir, $hours);


Thanks again for any help you can offer. Sorry if some of the long lines make the script kind of hard to read.

-Prime

Replies are listed 'Best First'.
Re: Code Review
by Ovid (Cardinal) on Dec 12, 2002 at 22:25 UTC

    This is a quick refactoring of your _get_list function. It's not guaranteed to work (it's untested) and it could use more refactoring, but it gives you the idea of what's going on. What I've done is move duplicate code into its own subroutine and taken code unrelated to the actual function and moved it into a subroutine that is (as far as I can tell) descriptively named. This makes the logic of the _get_list function much cleaner and easier to read.

    Again, this is only a rough start, but it gives you an idea of where to go from here. Note that the actual logic of _get_list has been altered slightly so as to avoid a bit of duplication.

    sub _get_list { my ($date, $report_dir, $config_dir, $hours) = @_; my %reports; if ($hours == 0) { %reports = _sum_report_entries($config_dir, 'report_master_list'); } else { %reports = _sum_report_entries($config_dir, 'report_temp_list'); if ($hours == 8) { _write_non_transferred_reports( %reports ); unlink "$config_dir/report_temp_list" or warn "$!"; return; # this *seems* to duplicate the logic! } _get_reports($date, $report_dir, $config_dir, \%reports); } sub _write_non_transferred_reports { my %reports = @_; open OUT, ">> /disk2/daily_report" or die "$!"; print OUT "\n\nDaily Reports That Were Unable To Be Transferred\n"; print OUT "------------------------------------------------\n\n"; for my $report (sort keys %reports) { print OUT "$report\n"; } print OUT "\n\n"; close OUT; } sub _sum_report_entries { my ($config_dir,$list_file) = @_ my $report = "$config_dir/$list_file"; my %sums; open REPORT, "<", $report or die "Cannot open ($report) for reading: + $!"; while ( chomp (my $line = <REPORT>) ) { $sums{$line}++; } close REPORT or warn "Cannot close ($report): $!"; return %sums; }

    Cheers,
    Ovid

    New address of my CGI Course.
    Silence is Evil (feel free to copy and distribute widely - note copyright text)

      Thanks for the reply! The changes you have suggested do make the code a lot more readable. Plus it has given me an idea on some similar changes in another script I am working on. Thanks again!

      -Prime
Re: Code Review
by chromatic (Archbishop) on Dec 12, 2002 at 22:53 UTC

    Can you tar the reports per-site and then copy only the tar file, unpacking it on the remote host?

      This is a great idea. The reason the script runs continually during the 8 hour period is because the reports all kick off at different times and they are consistent as to when they finish. But I can just as I see they are finished append them to the tar file and then on the final run at 8 move over the tar files. Thanks!

      -Prime

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://219440]
Approved by adrianh
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others having an uproarious good time at the Monastery: (4)
As of 2024-03-29 11:26 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found