Sub only grabbing first line from STDIN

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

Replies are listed 'Best First'.
Re: Sub only grabbing first line from STDIN by boo_radley (Parson) on May 14, 2002 at 14:26 UTC
`foreach $line (@text) { my $text =$text[$line];` [download] $line doesn't contain an index number of your array, it's the contents of each array element. That is, what you think is in `$text[$line]` is really in `$line` There may be other things wrong with the rest of the script (looks like there's more to it than this). use strict, use -w and if you need 'em, use diagnostics. update See below for other, more direct answers.	[reply] [d/l] [select]
Re: Sub only grabbing first line from STDIN by jmcnamara (Monsignor) on May 14, 2002 at 14:27 UTC
You are closing the file in the `foreach` loop. Apart from that there are a few other things that could be improved. Maybe like this: `#!/usr/bin/perl -w use strict; open DOM_WORK, '>/usr/spool/lpd/dom/dom.work' or die "Error messag +e here\n"; get_text(); close DOM_WORK; sub get_text { while (<>) { tr/\x0c-\x0d//d; print DOM_WORK $_; } get_domkey(); }` [download] -- John.	[reply] [d/l]
Re: Sub only grabbing first line from STDIN by Albannach (Monsignor) on May 14, 2002 at 14:33 UTC
All the problems noted so far would have been pointed out to you by Perl itself if you had `use warnings` (or -w if your Perl is older). You could also add some indentation in your loop to show the flow better, and some error checking (e.g. die if your open fails) would help you out too. For instance, if you `use warnings` you should get a message about printing to a closed filehandle, and also probably a message about `$text` not being numeric as an array index, which would point out the error that boo_radley noted above. You will also find that `use strict` helps immeasurably in making you think about your use of variables. -- I'd like to be able to assign to an luser	[reply] [d/l] [select]
Re: Sub only grabbing first line from STDIN by Biker (Priest) on May 14, 2002 at 14:29 UTC
You're closing DOM_WORK in your foreach loop in get_text(). Close it outside of get_text(). Update: jmcnamara, watch me when I install my new turbo charged keyboard... ;-) Everything went worng, just as foreseen.	[reply]
Re: Sub only grabbing first line from STDIN by tadman (Prior) on May 14, 2002 at 15:01 UTC
You are reading in the entire file, but are accessing it wrong. As others have pointed out, your program is a little mangled, but despite this, it is surprisingly close to working. It may not have been explained why you were getting the first line of the file, but not the others. Here's the reason: `my $text = $text[$line];` Here, because of the way you have structured your loop, `$line` contains a line from your file. This is then used to look up an entry in the `@text` array, so it has to be converted to a number. Since your text file probably doesn't have any numbers, this is always 0, which is the "first" line of your file. If your file had numbers in it, you would get quite a different result. Also note that even though you delete the control characters, you just print the same old line unmodified, instead of your fixed variable. Your whole program could probably be replaced with: `% perl -pi -e 'tr/\x0c-\x0d//d' /usr/spool/lpd/dom/dom.work` If you want to use it as a script, it has to be re-worked. A few notes: Don't reference your subroutines with ampersand. Just use them with brackets. `foo()` and not `&foo`. Indent your code properly, it makes it way more readable. You don't need to `select` your filehandle, just print to it: `print DOM_WORK "stuff"` Try and declare your subroutines before you use them. This backwards programming can cause subtle problems with order-of-operations, usually related to using variables before they are fully defined. Don't forget to trap errors if you can't `open`	[reply] [d/l] [select]
Re: Sub only grabbing first line from STDIN by demerphq (Chancellor) on May 14, 2002 at 15:36 UTC
Embarrassed Update Hmm, I really should learn to hit SUBMIT. :-) When I wrote this there was only Boo_Radleys reply, but I got called away to a meeting and apparently hit the wrong button as I left. So ive covered everybody elses points over again. Sorry about that. Weel, lets see, first lets indent this mess... `open DOM_WORK, '>/usr/spool/lpd/dom/dom.work'; &get_text; sub get_text{ my @text=(<>); my $line =0; foreach $line (@text) { my $text =$text[$line]; $text =~tr/\x0c-\x0d//d; select DOM_WORK; print "$text[$line]"; close DOM_WORK; } &get_domkey; }` [download] So whats wrong with this? lets see... `open DOM_WORK, '>/usr/spool/lpd/dom/dom.work'; &get_text;` [download] First you open a global filehandle, and you dont check to see what happens... The first is a minor nono in my book the second is negligent programming. Then you call a sub, one that isnt defined. I dont like that, but im a top subber. Personally I find that this leads to errors, others disagree. Ill post a link to a debate on the subject in a mo. Either way I think the following is much better `open my $DOM_WORK_FH, '>/usr/spool/lpd/dom/dom.work' or die "Open dom. +work failed $!"; get_text($DOM_WORK_FH); # you could also pass in the filename and open + the file in the sub...` [download] And then we get to the sub `sub get_text{ my @text=(<>); my $line =0; foreach $line (@text) {` [download] First the assignment of `<>` to @text means the whole file will be loaded into memory. Which probably is uneccesary, and if you get a large file will cause serious problems. Add to the fact that the add parens around the `<>` is distracting. Then you declare and initialize $line to be 0. Ok fine. Only thing is you then iterate over @text with $line. So first off the 0 goes bye-bye, but more importantly $line for each iteration contains a line of text, not the index of that line in the array. (Interestingly the will work once and once only, as the string will eval in the array to 0 and thus give you the first line in the array, but it would do that every time.) So we can probably clean up this mess into `sub get_text { my $out_fh=shift #what filehandle to write to while (<>) { # read a line at a time into $_` [download] So then we get to the processing of each line `my $text =$text[$line]; $text =~tr/\x0c-\x0d//d;` [download] The first assignment does nothing useful. in fact it blows the contents of line and would generate large quantities of warnings if you were using them. The second line does some basic character stripping, it seems ok. So this gets reduced to `tr/\x0c-\x0d//d; # strip unwanted chars from $_` [download] And then we get to the really interesting bit, `select DOM_WORK; print "$text[$line]"; close DOM_WORK;` [download] What does this do? The first line sets print to use the global filehandle that you opened earlier. The second line prints out what you think is the line, but really isnt. Even if it was you dont need to quote it. But the last line is of most interest: Did you really want to CLOSE your ouput file after the first line? No I didnt thinks so `print $out_fh $_;` [download] Which brings us to the end of the loop and the calling of another sub. Why are you chaining subs like this? Its confusing when you have a lot of subs. Better to call the second subroutine in the same place you call the first. Especially as you arent passing any info from one subroutine to the other. So in the end the rewritten code would be `use strict; use warnings; sub get_text{ my $output_file=shift; open my $out_fh,">".$output_file or die "Failed to open $output_file + because $!"; while (<>) { tr/\x0c-\x0d//d; print $out_fh $_; } } get_text('/usr/spool/lpd/dom/dom.work'); + get_domkey();` [download] Which I think youll agree is a lot easier to read, and actually works. boo_radley already said it, but almost all of the mistakes you made in this snippet would have been patently obvious had you turned warnings and strict on. Hope this points you in the right direction, Yves / DeMerphq --- Writing a good benchmark isnt as easy as it might look.	[reply] [d/l] [select]
Re: Sub only grabbing first line from STDIN by Zaxo (Archbishop) on May 14, 2002 at 15:06 UTC
"You're not incrementing $line" was my first impression, but in fact you have $line as the text you want, then you use it as an array index which probably evaluates to zero. To top it off, you assign to $line using that. Also, it's a good idea to open an close handles at the same level, and to check for errors on system interfaces like open. Your &get_text sub takes no arguments, and really doesnt seem to add much to the organisation. Here is a decrufted version: `use Fcntl qw(LOCK_EX); open DOM_WORK, '> /usr/spool/lpd/dom/dom.work' or die $!; LOCK: { flock DOM_WORK, LOCK_EX or die $!; } for (<>) { tr/\x0c-\x0d//d; print DOM_WORK or die $!; } close DOM_WORK or die $!; &get_domkey; # whatever that is` [download] I added locking in case of contention for the file. You may want to redo instead of dying, so I threw in the bareblock around that. If dom.work is some sort of queue, you probably want to open it to append ('>>') After Compline, Zaxo	[reply] [d/l]