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

can I get some help adding some error checking. when my source file CONF_ID has blank lines it gives me an error of uninitialized value in $confID and $confPIN, which is true, how would I test for this before running the rest of the code?

also are there any other suggestions on cleaning up the code?

And lastly, is there a more secure way to copy files to a server w/o putting the password in the perl file?

#!/usr/bin/perl # # Written By Jon # This script takes CONF_IN and populates # 3 templates which are then put into a final file all # together # Usage: perl conf_add.pl # requires: # input: templates in templates folder # input: list of conference IDs and conference PINs # with ID and PIN on one line in order. # program outputs to file: conference_additions # # TODO: Error handling when there are no entries in CONF_ID # TODO: SCP files to server # TODO: Random Number Generator # TODO: Add insert.pl use strict; use warnings; use File::Copy ("cp"); my($confINPUT); my($i) =0; #my($confOutName) = $ARGV[0]; my($date) = &getDate; open(CONF_ID, "<", "$ARGV[0]") || die("$!\n"); open(CONFLIST1, ">>", "conf_list1.tmp") || die("$!\n"); open(CONFLIST2, ">>", "conf_list2.tmp") || die("$!\n"); open(CONFLIST3, ">>", "conf_list3.tmp") || die("$!\n");; open(CONFADD, ">>", "./additions/additions\_$ARGV[0]\_$date") || die( +"$!\n"); while(<CONF_ID>){ $i++; open(TEMPLATE1, "<", "./templates/conf_template1") || die("$!\n") +; open(TEMPLATE2, "<", "./templates/conf_template2") || die("$!\n") +; open(TEMPLATE3, "<", "./templates/conf_template3") || die("$!\n") +; my($confINPUT) = $_; (my($confID), my($confPIN)) = split(" ",$confINPUT); while(<TEMPLATE1>){ $_ =~ s/XXXXXX/$confID/g; print CONFLIST1 $_; } while(<TEMPLATE2>){ $_ =~ s/XXXXXX/$confID/g; $_ =~ s/AAAA/$confPIN/g; print CONFLIST2 $_; } while(<TEMPLATE3>){ $_ =~ s/XXXXXX/$confID/g; print CONFLIST3 $_; } close(TEMPLATE1) || die("$!\n"); close(TEMPLATE2) || die("$!\n"); close(TEMPLATE3) || die("$!\n"); } # close files for input close(CONFLIST1) || die("$!\n"); close(CONFLIST2) || die("$!\n"); close(CONFLIST3) || die("$!\n"); # open files for output open(CONFLIST1, "<", "conf_list1.tmp") || die("$!\n"); open(CONFLIST2, "<", "conf_list2.tmp") || die("$!\n"); open(CONFLIST3, "<", "conf_list3.tmp") || die("$!\n"); # adds 1st set of conf config to conf_additions print CONFADD ("; start conference config 1\n"); while(<CONFLIST1>){ print CONFADD $_; } # adds 2nd set of conf config to conf_additions print CONFADD ("\n\n; start conference config 2\n"); while(<CONFLIST2>){ print CONFADD $_; } # adds 3rd set of conf config to conf_additions print CONFADD ("\n\n; meetme_additional_custom.conf\n"); while(<CONFLIST3>){ print CONFADD $_; } close(CONFADD); # closes temporary files close (CONFLIST1) || die("$!\n"); close (CONFLIST2) || die("$!\n"); close (CONFLIST3) || die("$!\n"); # moves list to finished folder cp("$ARGV[0]", "./finished_list/$ARGV[0].finished"); unlink("$ARGV[0]"); # deletes temporary files unlink("conf_list1.tmp") || die("$!\n"); unlink("conf_list2.tmp") || die("$!\n"); unlink("conf_list3.tmp") || die("$!\n"); print("Number of Entries processed: $i\n"); exit(0); sub getDate(){ my(@months) = qw(Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec); my(@weekDays) = qw(Sun Mon Tue Wed Thu Fri Sat Sun); my($second, $minute, $hour, $dayOfMonth, $month, $yearOffset, $day +OfWeek, $dayOfYear, $daylightSavings) = localtime(); my($year) = 1900 + $yearOffset; my($theTime) = "$dayOfMonth\_$months[$month]"; return $theTime; }

Replies are listed 'Best First'.
Re: Error Correction
by almut (Canon) on Apr 22, 2010 at 21:56 UTC
    it gives me an error of uninitialized value in $confID and $confPIN

    Generally, you can use defined to test whether a variable holds a defined (initialized) value.

    And depending on what you want to happen in that case, you could either skip the rest of the loop body

    while (<CONF_ID>){ my ($confID, $confPIN) = split ' '; next unless defined $confID; ... }

    or set the variables to some defined but empty value, e.g.

    while (<CONF_ID>){ my ($confID, $confPIN) = split ' '; $confID = '' unless defined $confID; ... }

    just in case you wanted to substitute XXXXXX with nothing, etc., when there's an empty line in CONF_ID...

Re: Error Correction
by chromatic (Archbishop) on Apr 22, 2010 at 21:48 UTC

    If skipping blank lines is okay, use next at the top of the while loop:

    while(<CONF_ID>){ next unless /\S/; ... }

    That is, unless the line contains a non-whitespace character, jump to the next iteration of the loop.

    As for cleaning up your code, you have a lot of duplication and near duplication, especially reading your templates. If you factored that code out into a subroutine, you could make your program shorter. Of course, you could also skip re-reading the templates for every line of input, which would make your program faster.

    To do that, you need to extract other subroutines which only do one thing: read a file into a data structure, perform a substitution for every line of a data structure, write out a data structure to a file. Your code will be easier to read and modify because each individual unit has a descriptive name and a single, identifiable purpose.

    Style-wise, eschew global filehandles in favor of lexical filehandles and get rid of the prototype on getDate(). If you do the latter, you can drop the Perl 4-style invocation. Other than that, you use more parentheses than I would, which is mostly a matter of familiarity with Perl, so don't worry about that for now.

Re: Error Correction
by amedico (Sexton) on Apr 23, 2010 at 07:00 UTC
    1. Don't need parens when using "my" to declare a single variable - e.g.

      my $confINPUT;

      not:

      my($confINPUT);

      Similarly, do

      my ($confID, $confPIN)

      instead of:

      (my($confID), my($confPIN))
    2. Don't escape characters that don't need escaping, like the underscores at line ~33.
    3. You're inconsistent with operations that you check for failure. AFAIK it's pointless to check close() for failure when you've opened a file read-only. On the other hand, you aren't checking the result of your print() calls, which could fail.
    4. "my $confINPUT" at approx. line 24 (file scope) is unused (shadowed by the same-named variable inside the while loop) - eliminate it.
    5. Don't double-quote things that don't need string interpolation, like "$ARGV[0]". Just write $ARGV[0] directly.
    6. Some of your comments don't match the code, like # close files for input and # open files for output.
    7. Be aware that terminating your "die" messages with a newlines tells perl to not print the line number where the die occurs. This may be desirable if you want simpler error messages for end-users, but for debugging it's usually nice to see the line number.
    8. You copy and unlink the input file while you still have an open file handle (CONF_ID) to it. *nix lets you do this, but it won't work on Windows.
    9. You should check the result of the File::Copy copy() call. If the copy fails in your current code, you still blow away the original input file.
    I'd refactor to something like this:
    #!/usr/bin/perl # Written By Jon # This script takes CONF_IN and populates # 3 templates which are then put into a final file all # together # Usage: perl conf_add.pl # requires: # input: templates in templates folder # input: list of conference IDs and conference PINs # with ID and PIN on one line in order. # program outputs to file: conference_additions # # TODO: Error handling when there are no entries in CONF_ID # TODO: SCP files to server # TODO: Random Number Generator # TODO: Add insert.pl use strict; use warnings; use File::Copy ("cp"); use POSIX; my $TEMPLATES_DIR = "templates"; my $FINISHED_DIR = "finished_list"; my $OUTPUT_DIR = "additions"; my $date = POSIX::strftime('%d_%b', localtime); open(my $confid_fh, "<", $ARGV[0]) || die("$!\n"); my $i = 0; while (my $line = <$confid_fh>) { $i++; my ($confID, $confPIN) = split(" ", $line); process_template("conf_template1", "conf_list1.tmp", { XXXXXX => $ +confID }); process_template("conf_template2", "conf_list2.tmp", { XXXXXX => $ +confID, AAAA => $confPIN }); process_template("conf_template3", "conf_list3.tmp", { XXXXXX => $ +confID }); } close $confid_fh; open(my $confadd_fh, ">>", $OUTPUT_DIR . "/additions_$ARGV[0]_$date") +|| die("$!\n"); # adds 1st set of conf config to conf_additions print $confadd_fh "; start conference config 1\n"; append("conf_list1.tmp", $confadd_fh); # adds 2nd set of conf config to conf_additions print $confadd_fh "\n\n; start conference config 2\n"; append("conf_list2.tmp", $confadd_fh); # adds 3rd set of conf config to conf_additions print $confadd_fh "\n\n; meetme_additional_custom.conf\n"; append("conf_list3.tmp", $confadd_fh); close $confadd_fh; # moves list to finished folder cp($ARGV[0], $FINISHED_DIR . "/$ARGV[0].finished"); unlink $ARGV[0]; # deletes temporary files unlink "conf_list1.tmp" || die("$!\n"); unlink "conf_list2.tmp" || die("$!\n"); unlink "conf_list3.tmp" || die("$!\n"); print "Number of Entries processed: $i\n"; exit 0; sub process_template { my ($tmplfile, $outfile, $substitutions_ref) = @_; open(my $out_fh, ">>", $outfile) || die("$!\n"); open(my $template_fh, "<", $TEMPLATES_DIR . "/" . $tmplfile) || di +e("$!\n"); while (my $line = <$template_fh>) { while (my ($key, $value) = each %{$substitutions_ref}) { $_ =~ s/$key/$value/g; } print $out_fh $_; } close $template_fh; close $out_fh || die("$!\n"); } sub append { my ($infile, $out_fh) = @_; open(my $in_fh, "<", $infile) || die("$!\n"); while (<$in_fh>) { print $out_fh $_; } close $in_fh; }
Re: Error Correction
by toolic (Bishop) on Apr 23, 2010 at 00:36 UTC
    also are there any other suggestions on cleaning up the code?
    Your getDate function can be replaced with POSIX::strftime
    use POSIX; my $date = POSIX::strftime('%d_%b', localtime);

    If you turn your block comments at the top of your code into POD, you'll get a manpage for free using perldoc.

    =pod This script takes CONF_IN and populates 3 templates which are then put into a final file all together Usage: perl conf_add.pl requires: input: templates in templates folder input: list of conference IDs and conference PINs with ID and PIN on one line in order. program outputs to file: conference_additions =cut

    perlcritic is a tool which can offer other best practices.

Re: Error Correction
by Khen1950fx (Canon) on Apr 22, 2010 at 23:12 UTC
    I tried using the suggestions by chromatic and almut:
    #!/usr/bin/perl use strict; use warnings; use File::Copy "cp"; my $confINPUT; my ($i) = 0; my ($date) = &getDate; my ($confOutName) = shift @ARGV; die "$!\n" unless open CONF_ID, '<', "@ARGV"; die "$!\n" unless open CONFLIST1, '>>', 'conf_list1.tmp'; die "$!\n" unless open CONFLIST2, '>>', 'conf_list2.tmp'; die "$!\n" unless open CONFLIST3, '>>', 'conf_list3.tmp'; die "$!\n" unless open CONFADD, '>>', "./additions/additions_$ARGV[0 +]_date"; while ( defined( $_ = <CONF_ID> ) ) { next unless /\S/; die "$!\n" unless open TEMPLATE1, '<', './templates/conf_template' +; die "$!\n" unless open TEMPLATE2, '<', './templates/conf_template2 +'; die "$!\n" unless open TEMPLATE3, '<', './templates/conf_template3 +'; $confINPUT = $_; my ( $confID, $confPIN ) = split( " ", $confINPUT, 3 ); while ( defined( $_ = <TEMPLATE1> ) ) { $_ =~ s/XXXXXX/$confID/g; print CONFLIST1 $_; } while ( defined( $_ = <TEMPLATE2> ) ) { $_ =~ s/XXXXXX/$confID/g; $_ =~ s/AAAA/$confPIN/g; print CONFLIST2 $_; } while ( defined( $_ = <TEMPLATE3> ) ) { $_ =~ s/XXXXXX/$confID/g; print CONFLIST3 $_; } die "$!\n" unless close TEMPLATE1; die "$!\n" unless close TEMPLATE2; die "$!\n" unless close TEMPLATE3; } die "$!\n" unless close CONFLIST1; die "$!\n" unless close CONFLIST2; die "$!\n" unless close CONFLIST3; die "$!\n" unless open CONFLIST1, '<', 'conf_list1.tmp'; die "$!\n" unless open CONFLIST2, '<', 'conf_list2.tmp'; die "$!\n" unless open CONFLIST3, '<', 'conf_list3.tmp'; print CONFADD "; start conference config 1\n"; while ( defined( $_ = <CONFLIST1> ) ) { print CONFADD $_; } print CONFADD "\n\n; start conference config 2\n"; while ( defined( $_ = <CONFLIST2> ) ) { print CONFADD $_; } print CONFADD "\n\n; meetme_additional_custom.conf\n"; while ( defined( $_ = <CONFLIST3> ) ) { print CONFADD $_; } close CONFADD; die "$!\n" unless close CONFLIST1; die "$!\n" unless close CONFLIST2; die "$!\n" unless close CONFLIST3; cp( "$ARGV[0]", "./finished_list/@ARGV.finished" ); unlink "$ARGV[0]"; die "$!\n" unless unlink 'conf_list1.tmp'; die "$!\n" unless unlink 'conf_list2.tmp'; die "$!\n" unless unlink 'conf_list3.tmp'; print "Number of Entries processed: $i\n"; exit 0; sub getDate() { my (@months) = qw(Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov De +c); my (@weekDays) = qw(Sun Mon Tue Wed Thu Fri Sat Sun); my ( $second, $minute, $hour, $dayOfMonth, $month, $yearOffset, $dayOfWeek, $dayOfYear, $daylightSavings ) = localtime(); my ($year) = 1900 + $yearOffset; my ($theTime) = "${dayOfMonth}_$months[$month]"; return $theTime; }
Re: Error Correction
by apl (Monsignor) on Apr 23, 2010 at 11:36 UTC
    In addition to everything else, you should replace (for example)

    open(CONFLIST1, ">>", "conf_list1.tmp")  || die("$!\n");

    with

    open(CONFLIST1, ">>", "conf_list1.tmp")  || die("Opening conf_list1.tmp $!\n");

    That is, include the path of the file you're trying to open. In the above case, it's trivial (or unnecessary), but if you use an array of paths, or a subroutine, you'll definitely want to know which file you had problems with. It also eliminates the problem (when constructing pathnames) of thinking you were using one path, when you actually were using something else entirely.