CGI/Perl Guide | Learning Center | Forums | Advertise | Login
Site Search: in

  Main Index MAIN
INDEX
Search Posts SEARCH
POSTS
Who's Online WHO'S
ONLINE
Log in LOG
IN

Home: Perl Programming Help: Beginner:
Advise for a newbie

 



savo
User

Sep 11, 2009, 1:08 PM

Post #1 of 12 (1794 views)
Advise for a newbie Can't Post

 
I have just read learning Perl 5th edition twice and tried a few other beginning Perl books and was finding them hard to get into so thought i would try just writing code and going that way for a while. I have written my first script and was wondering if someone with more knowledge than me could let me know if i am going about things in the right way or weather i should go back to reading so i don't make things harder for my self later on. If you feel i need to read some more any advise on books would be good. Also if any hints on where i am going wrong would be appreciated.


Code
#!/usr/bin/perl 
use warnings;
use strict;
use 5.010;

my @shows = ""; #List of all abailable shows
my @results = ""; #List of options to download after searching @shows

say "Please enter the show you would like to find. This can take a while as it will download all listings.\nTo exit please leave a blank line.";
chomp (my $lookup = <>);
#exit if there is a blank line
if ($lookup =~ /\^\s*$/){
say "Goodbye";
exit;}
# Downlad itv program list
open ITV, "get_iplayer --type='podcast,itv'|" or die "fork: $!";
while (<ITV>) {
push @shows, $_
}
#Download bbc list
open BBC, "get_iplayer |" or die "fork: $!";
while (<BBC>){
push @shows, $_
}
{ #push the results to new list
my $c =1; #used to count the printed shows
for (@shows) {
if (/$lookup/i) {
push @results, $_;
}
}
shift @results; #Removes empty value
for (@results) {
print $c, ") $_";
$c++;
}
}
{ # choose the program you want to download from results of serch
say "Please select a show to download. This could take a while as it will download the show selected.\n Type exit or leave a blank line to exit";
chomp (my $n = <STDIN>);
#exit on blank line or non number
if ($n =~ /\D|^\s*$/){
say "Goodbye";
exit;}
my @download = split /:/,$results[$n -1];
my $chan = "";
if ($download[1] =~ /itv/i) {
$chan = "ITV"
} else {
$chan = "tv"
}
open DOWN, "get_iplayer --type=$chan --get $download[0]|" or die "fork: $!";
while (<DOWN>) {
print $_;
}
}


Thanks

Sav


FishMonger
Veteran / Moderator

Sep 12, 2009, 2:11 PM

Post #2 of 12 (1772 views)
Re: [savo] Advise for a newbie [In reply to] Can't Post

It's good to see that you used the warnings and strict pragmas. It's rare to see a Perl beginner use those pragmas.

The rest of the code is on the right track, but could be improved.

You need to add proper vertical white space so that your code is easier to read/understand.

Your indentation is inconsistent and in some areas wrong.

You should keep the line lengths below 80 chars, if possible.

The 'or die' statement on the open calls won't work as written when opening a pipe. You need to check the return code, which should be 0 on success.

The redundant looping over the data and creating 2 arrays is inefficient and unnecessary. You only need 1 array and it should contain only the desired results.

The if/else block used to assign $chan would be better done with the ternary operator.

Here's a reworked (untested) version.


Code
#!/usr/bin/perl 

use warnings;
use strict;
use 5.010;

$|++;

my @shows; #List of all abailable shows

say "Please enter the show you would like to find.\n",
"This can take a while as it will download all listings.\n",
"To exit please leave a blank line.";

chomp (my $lookup = <>);
say "Goodbye" and exit if $lookup =~ /^\s*$/;

# Downlad itv program list
unless( open(ITV, '-|', "get_iplayer --type='podcast,itv'") == 0 ) {
die "fork: $!";
}
while (<ITV>) {
push @shows, $_ if /$lookup/i;
}

#Download bbc list
unless( open(BBC, '-|', "get_iplayer") ==0 ) {
die "fork: $!";
}
while (<BBC>){
push @shows, $_ if /$lookup/i;
}

my $c; #used to count the printed shows
say ++$c, ") $_" for @shows;

# choose the program you want to download from results of search
say "Please select a show to download.\n",
"This could take a while as it will download the show selected.\n",
"Type exit or leave a blank line to exit";

chomp (my $num = <>);
say "Goodbye" and exit if $num =~ /\D/;

my @download = split /:/,$shows[$num -1];
my $chan = ($download[1] =~ /itv/i) ? "ITV" : "tv";

unless( open(DOWN, '-|', "get_iplayer --type=$chan --get $download[0]") == 0) {
die "fork: $!";
}
print <DOWN>;



toolic
User

Sep 12, 2009, 2:27 PM

Post #3 of 12 (1771 views)
Re: [savo] Advise for a newbie [In reply to] Can't Post

I believe you have a minor bug in this line of code:

Code
if ($lookup =~ /\^\s*$/){


You do not want to escape the '^'. I believe this is what
you want:

Code
if ($lookup =~ /^\s*$/){


The layout is a little tough to follow. You could
download and run your code through perltidy:
http://perltidy.sourceforge.net/

Since you are collecting all of the output from
external programs, I think it is more conventional
to use backticks rather than opening a pipe. Using
a pipe would be more appropriate if you were filtering
the output, in order to use less memory in the program.
For example:


Code
my $cmd = q{get_iplayer --type='podcast,itv'}; 
my @shows = `$cmd`;
die "Error: $? for command $cmd" if $?;


You may want to chomp your array to remove newlines.

Otherwise, pretty good for a first script!


savo
User

Sep 14, 2009, 5:19 AM

Post #4 of 12 (1739 views)
Re: [toolic] Advise for a newbie [In reply to] Can't Post

I spotted the bug when i borrowed that line for another script and have now corrected it.

I tried back quotes at first but that wasn't working so i changed it and later found that it was my command at fault not the back quotes didn't even think to change it back was just very pleased with my self that it worked. I will try changing it back now.

Thanks for the link i wanted something like that to see how my code should look.

get_iplayer is a very handy program for anyone in the UK not using Windows to catch up on the tv. My little script also means that even my gf can look for and download a program easily.

Thanks again

Sav


shawnhcorey
Enthusiast


Sep 14, 2009, 5:32 AM

Post #5 of 12 (1737 views)
Re: [FishMonger] Advise for a newbie [In reply to] Can't Post


In Reply To
The 'or die' statement on the open calls won't work as written when opening a pipe. You need to check the return code, which should be 0 on success.


From `perldoc -f open`: Open returns nonzero upon success, the undefined value otherwise. If the "open" involved a pipe, the return value happens to be the pid of the subprocess.

Which means the "or die" will work even on a pipe. The difference is that for a pipe, it's the child's pid; for anything else, it's an undetermined non-zero value. But it's always "undef" if it fails.


Code
# Downlad itv program list   
open my $itv_fh, '-|', "get_iplayer --type='podcast,itv'" or die "could not fork to 'get_iplayer --type='podcast,itv': $!\n";
while( <$itv_fh> ){
push @shows, $_ if /$lookup/i;
}
close $itv_fh or die "could not close 'get_iplayer --type='podcast,itv': $!\n"


It's always a good idea to close your file handles so you can check for errors.

__END__

I love Perl; it's the only language where you can bless your thingy.

Perl documentation is available at perldoc.perl.org. The list of standard modules and pragmatics is available in perlmodlib.

Get Markup Help. Please note the markup tag of "code".


savo
User

Sep 14, 2009, 7:26 AM

Post #6 of 12 (1735 views)
Re: [FishMonger] Advise for a newbie [In reply to] Can't Post

Sorry FishMonger completely missed your post some how.

I will take another look tonight and see if i can get the back quotes working and get it all into one array.

I have installed perltidy now and ran that so will keep an eye on where i am going wrong on spacing.

Thanks again

Sav


FishMonger
Veteran / Moderator

Sep 14, 2009, 8:06 AM

Post #7 of 12 (1731 views)
Re: [shawnhcorey] Advise for a newbie [In reply to] Can't Post

Shawn, the keys words you missed in my statement was "as written" and as you can seen, your example fails to die.

C:\test>type test.pl

Code
#!/usr/bin/perl 

use strict;
use warnings;

open my $itv_fh, '-|', "get_iplayer --type='podcast,itv'"
or die "could not fork to 'get_iplayer --type='podcast,itv': $!\n";

print "Oops...it should have died, but didn't\n\n";

unless( open( my $ITV, '-|', "get_iplayer --type='podcast,itv'") == 0 ) {
die "fork: $!";
}


C:\test>test.pl

Quote
Oops...it should have died, but didn't

fork: No such file or directory at C:\test\test.pl line 12.
'get_iplayer' is not recognized as an internal or external command,
operable program or batch file.
'get_iplayer' is not recognized as an internal or external command,
operable program or batch file.


I do agree that it's a good idea to check the return of the close statement, but if the open failed, why even bother to proceed with the while loop and close statement?


(This post was edited by FishMonger on Sep 14, 2009, 8:17 AM)


shawnhcorey
Enthusiast


Sep 14, 2009, 8:34 AM

Post #8 of 12 (1726 views)
Re: [FishMonger] Advise for a newbie [In reply to] Can't Post


In Reply To
Shawn, the keys words you missed in my statement was "as written" and as you can seen, your example fails to die.


Then there's a bug in Perl. The open should return the process id when opening a pipe.

(light bulb) The problem is you are running under Windows. Windows' processes do have identifiers but Perl ignores them. Instead, it thinks all the pids are zero. :(

Try:

Code
defined( open my $itv_fh, ... ) or die "...";


Great, now I have to change all my open statements. :)

__END__

I love Perl; it's the only language where you can bless your thingy.

Perl documentation is available at perldoc.perl.org. The list of standard modules and pragmatics is available in perlmodlib.

Get Markup Help. Please note the markup tag of "code".


FishMonger
Veteran / Moderator

Sep 14, 2009, 8:59 AM

Post #9 of 12 (1722 views)
Re: [shawnhcorey] Advise for a newbie [In reply to] Can't Post


In Reply To
(light bulb) The problem is you are running under Windows. Windows' processes do have identifiers but Perl ignores them. Instead, it thinks all the pids are zero. :(

Try:

Code
defined( open my $itv_fh, ... ) or die "...";



It's not a Windows problem.

Lets see what happens on Linux.

Code
[root@fc4dev ~]# cat test.pl 
#!/usr/bin/perl

use strict;
use warnings;

open my $itv_fh, '-|', "get_iplayer --type='podcast,itv'"
or die "could not fork to 'get_iplayer --type='podcast,itv': $!\n";

print "Oops...it should have died, but didn't\n\n";

unless( open( my $ITV, '-|', "get_iplayer --type='podcast,itv'") == 0 ) {
die "fork: $!";
}


Quote
[root@fc4dev ~]# ./test.pl
sh: get_iplayer: command not found
Oops...it should have died, but didn't

fork: Illegal seek at ./test.pl line 12.
sh: get_iplayer: command not found


I get the exact same results when using defined as you suggest.


shawnhcorey
Enthusiast


Sep 14, 2009, 9:18 AM

Post #10 of 12 (1719 views)
Re: [FishMonger] Advise for a newbie [In reply to] Can't Post


In Reply To
It's not a Windows problem.

Lets see what happens on Linux.


Strange, I get this:


Code
#!/usr/bin/perl 

use strict;
use warnings;

use Data::Dumper;
# Make Data::Dumper pretty
$Data::Dumper::Sortkeys = 1;
$Data::Dumper::Indent = 1;
$Data::Dumper::Maxdepth = 0;


test_open( "cat $0" );
test_open( "foobar $0" );

sub test_open {
my $cmd = shift @_;

print "for: $cmd\n";

my $x = ( open my $fh, '-|', $cmd );
print Dumper \$x, \$fh;
close $fh;

open $fh, '-|', $cmd or die "could not open $cmd: $!\n";
print "open worked\n";
close $fh;

print "\n\n";
}



Quote
for: cat ./pl
$VAR1 = \2415;
$VAR2 = \\*{'::$fh'};
open worked


for: foobar ./pl
Can't exec "foobar": No such file or directory at ./pl line 21.
$VAR1 = \undef;
$VAR2 = \\*{'::$fh'};
Can't exec "foobar": No such file or directory at ./pl line 25.
could not open foobar ./pl: No such file or directory


__END__

I love Perl; it's the only language where you can bless your thingy.

Perl documentation is available at perldoc.perl.org. The list of standard modules and pragmatics is available in perlmodlib.

Get Markup Help. Please note the markup tag of "code".


savo
User

Sep 14, 2009, 11:18 AM

Post #11 of 12 (1715 views)
Re: [shawnhcorey] Advise for a newbie [In reply to] Can't Post

This is taken from Learning Perl and is what i based mine on.


Code
open F, "find / -atime +90 -size +1000 -print|" or die "fork: $!"; 
while (<F>) {
chomp;
printf "%s size %dK last accessed on %s\n",
$_, (1023 + -s $_)/1024, -A $_;
}



shawnhcorey
Enthusiast


Sep 14, 2009, 1:12 PM

Post #12 of 12 (1711 views)
Re: [savo] Advise for a newbie [In reply to] Can't Post

Well, I don't understand why the code you supplied doesn't die on success but after a little digging, I can only say opening a pipe is a strange beast. Here is a list of perldocs I discovered, so far, that has bearing on it:

perlodc -f open
perldoc -f fork
perldoc -f exec
perldoc perlopentut
perldoc perlipc

When you open a pipe, Perl does a fork and exec. From `perldoc -f exec`: If there is only one scalar argument or an array with one element in it, the argument is checked for shell metacharacters, and if there are any, the entire argument is passed to the systemís command shell for parsing (this is "/bin/sh -c" on Unix platforms, but varies on other platforms). If there are no shell metacharacters in the argument, it is split into words and passed directly to "execvp", which is more efficient.

This means if there is a shell metacharacter in the argument, a shell is launched and its PID is returns even if the command fails!

This means that you won't know if the command failed unless you check for errors on the close.

__END__

I love Perl; it's the only language where you can bless your thingy.

Perl documentation is available at perldoc.perl.org. The list of standard modules and pragmatics is available in perlmodlib.

Get Markup Help. Please note the markup tag of "code".

 
 


Search for (options) Powered by Gossamer Forum v.1.2.0

Web Applications & Managed Hosting Powered by Gossamer Threads
Visit our Mailing List Archives