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

Hey all- here is the code... it is pretty sloppy i am sure, but it is also my first time. Perl is still very new to me.

#!/usr/bin/perl -w print "What is input file name?: "; chomp($input = <STDIN>); print "What is output file name: "; chomp($output = <STDIN>); print "What is the mininum magnitude?:"; chomp($min_mag = <STDIN>); print "What is maximum magnitude?:"; chomp($max_mag = <STDIN>); open (INPUT, "$input") || die "died opening input\n";#opening up ngc18 +8cata and putting it into INPUT# open (STDOUT, ">$output") || die "died opening output\n"; #opening up +ngc188cat2 and setting output of out# $cfo = "O"; $status = "STATUS=OK"; open OUT, "+<holder"; while ($inline = <INPUT>) { #INPUT defined as variable# @mag = split(/\s+/, $inline); #firstline of array defined by split of + white space into variable inline# if ($mag[12] >= $min_mag && $mag[12] <= $max_mag) { print OUT "$inline"; #printing to OUT } } close OUT; open NEW, "+<holder"; while ($sep = <NEW>) { @firstline = split(/\s+/, $sep); $hms1 = $firstline[2]; $rah2 = ($hms1 / 15); $rah3 = int($rah2); $ram1 = ( abs($rah2 - $rah3) )* 60; $ram2 = int($ram1); $ras1 = abs($ram1 - $ram2) * 60; $ras2 = sprintf "%-02.3f", $ras1; ############################################ $hms2 = $firstline[3]; $hh2 = sprintf "%1d", ($hms2); $mm2 = sprintf "%02d", (60*( abs($hms2) - abs($hh2) ) ); $ss2 = sprintf "%-02.2f", 3600*abs($hms2) - 3600*abs($hh2) - 60*abs($ +mm2); ############################################################ $obid = "NGC 188 - $firstline[1]"; ########################################################### write; #writing to STDOUT } close INPUT; close NEW; format STDOUT = @>>> @<<<<<<<<<<<<<<<<<<< @> @> @>>>>> @< @< @>>>> @ @<<<<<<<< $firstline[1], $obid, $rah3, $ram2, $ras2, $hh2, $mm2, $ss2, $cfo, $st +atus, .
Just to explain how i thought about this. The information is ran through the first loop, containing the "if" statement, which takes out the information I want. From there, that information is put into a seperate external file. That external file is then opened and ran through a second loop, which gives me the information i need, that is subsquently formatted into a data report.

So what i am wondering is how i can clean up the loops, or if it is possible and easy combine the two loops that I have in order to make just one loop? I would perfer to not have an external file ("holder") if at all possible. I would appericate some help, because I am going to have to make some more if loops like this, asking to examine different elements in the original dataset instead.

Thank you! srkaeppler

Replies are listed 'Best First'.
Re: Making two loops into one
by BrowserUk (Patriarch) on Sep 21, 2004 at 21:23 UTC

    Without getting into testing, it look very much as if you could simply move the body of the second while loop into the body of the if statement in the first; skip the holder file stuff and rename @firstline* to @mag to match the usage in the first loop.

    * Bad name. It's an array not a line and it it used by every line, not just the first??

    Untested (no data), but something along these lines should do it.

    #!/usr/bin/perl -w print "What is input file name?: "; chomp($input = <STDIN>); print "What is output file name: "; chomp($output = <STDIN>); print "What is the mininum magnitude?:"; chomp($min_mag = <STDIN>); print "What is maximum magnitude?:"; chomp($max_mag = <STDIN>); #opening up ngc188cata and putting it into INPUT# open (INPUT, "$input") || die "died opening input\n"; #opening up ngc188cat2 and setting output of out# open (STDOUT, ">$output") || die "died opening output\n"; $cfo = "O"; $status = "STATUS=OK"; while ($inline = <INPUT>) { #INPUT defined as variable# #firstline of array defined by split of white space into variable +inline @mag = split(/\s+/, $inline); if ($mag[12] >= $min_mag && $mag[12] <= $max_mag) { $hms1 = $mag[2]; $rah2 = ($hms1 / 15); $rah3 = int($rah2); $ram1 = ( abs($rah2 - $rah3) )* 60; $ram2 = int($ram1); $ras1 = abs($ram1 - $ram2) * 60; $ras2 = sprintf "%-02.3f", $ras1; $hms2 = $mag[3]; $hh2 = sprintf "%1d", ($hms2); $mm2 = sprintf "%02d", (60*( abs($hms2) - abs($hh2) ) ); $ss2 = sprintf "%-02.2f", 3600*abs($hms2) - 3600*abs($hh2) - 60*abs($mm2); $obid = "NGC 188 - $mag[1]"; write; #writing to STDOUT } } close INPUT; close NEW; format STDOUT = @>>> @<<<<<<<<<<<<<<<<<<< @> @> @>>>>> @< @< @>>>> @ @<<<<<<<< $mag[1], $obid, $rah3, $ram2, $ras2, $hh2, $mm2, $ss2, $cfo, $status, .

    Examine what is said, not who speaks.
    "Efficiency is intelligent laziness." -David Dunham
    "Think for yourself!" - Abigail
    "Memory, processor, disk in that order on the hardware side. Algorithm, algorithm, algorithm on the code side." - tachyon
Re: Making two loops into one
by graff (Chancellor) on Sep 22, 2004 at 04:20 UTC
    I think your basic question is solved, and ysth has warned you about a possible risk with your open() statements. I have a slightly different admonishment about getting the file names (and min/max params):

    Use @ARGV, and get the file names, and min/max values as well, from the command line, instead of forcing the user into a dialog with printed prompts and keyboard input via STDIN. You can look at the Getopt::Long and Getopt::Std modules to see how most people do this sort of thing, or you can easily "roll your own" -- something like this:

    my $Usage = "Usage: $0 --min MIN --max MAX --in INFILE --out OUTFILE\n +"; my %args; while ( @ARGV >= 2 ) { if ( $ARGV[0] =~ /^--(min|max|in|out)$/ ) { my $opt = substr( shift, 2 ); $args{$opt} = shift; } else { last; } } die $Usage if ( @ARGV or ( keys %args != 4 )); # all 4 args are mandatory, and only these 4 args are allowed # now use $args{in} instead of $input, etc...
    (But really, the Getopt modules are worthwhile -- reading their man pages is time well spent.)

    There are several reasons for using command-line args in @ARGV instead of a runtime dialog with the user:

    • Most command line shells provide a "command history" feature that lets you recall, modify and re-execute prior commands; your program will be a lot easier and quicker to use if it gets everything it needs from the command line.
    • Most shells provide pretty good line-editing functions (moving back and forth on the line before executing the command, deleting / cutting / pasting characters or words, etc) whereas the handling of STDIN within a perl script may provide very limited editing abilities (unless you learn about and use a suitable Term module).
    • On the command line, you can alter/correct any arg at any time before starting execution; but if you're typing in single answers to questions at runtime and notice a mistake after entering any one of the answers, you just have to quit and start over (very frustrating).
    • If the program gets everything it needs from the command line, you can "automate" it -- i.e. run it from some other script; but if you have to carry on a dialog after it starts, running it via some other script becomes a fairly complicated and tricky business.
    • (update -- forgot to mention:) Many shells provide "file name completion" on the command line, whereby you simply type the first few characters of a name, hit the tab key or something, and the rest of the file name is filled in for you (or you get a list of files that match what you've typed so far); again, you would only get this feature within a perl script if you learned and used one of the Term modules (which I haven't done yet, because @ARGV handles pretty much everything I ever need).

    Note that the ordering of arg flags is flexible: "--min 0" can come before or after "--max 10", and likewise for the file name args; all that matters is that each value comes immediately after its flag.

Re: Making two loops into one
by ysth (Canon) on Sep 21, 2004 at 23:17 UTC
    chomp($input = <STDIN>); ... chomp($output = <STDIN>); ... open (INPUT, "$input") || die "died opening input\n";#opening up ngc18 +8cata and putting it into INPUT# open (STDOUT, ">$output") || die "died opening output\n"; #opening up +ngc188cat2 and setting output of out#
    Do not use user-input filenames this way unless you place full trust in your user (and even then, it's a lousy habit to develop); this form of open can be used to run arbitrary processes (e.g. $input = "rm -rf /some/critical/directory |"). Use three arg open instead, or explicitly specify the mode and a space, e.g.:
    open(INPUT, "<", $input) or die ... open(OUTPUT, ">", $output) or die ... or open(INPUT, "< $input") or die ... open(OUTPUT, "> $output") or die ...
Re: Making two loops into one
by Roy Johnson (Monsignor) on Sep 21, 2004 at 21:22 UTC
    It looks like you can just do everything you do in the second loop, within the if-block of the first loop.

    Caution: Contents may have been coded under pressure.