Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl Monk, Perl Meditation
 
PerlMonks  

getting a while loop to terminate

by Aldebaran (Curate)
on Apr 15, 2012 at 02:16 UTC ( [id://965118]=perlquestion: print w/replies, xml ) Need Help??

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

Hello good people

I've glanced at other people's posts and must say that this is very low-brow in comparison to what others are able to do. I guess we all start somewhere. Most of this script works for me, but I haven't quite wrangles the subroutine to return a new directory, in particular, what happens is that the while loop never terminates:

$ perl tg1.pl Useless use of private variable in void context at tg1.pl line 33. site_138 site_139 site_140 site_141 ^C $ cat tg1.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new; $m->get( $domain); my $counter = 0; my @list = $m->images(); for my $img (@list) { my $url = $img->url_abs(); $counter++; my $dir = &dirname; my $filename = "$dir". "/image_". "$counter"; getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub dirname { my $word = "site"; my $counter = 1; my $flag = 1; while ($flag) { my $name = "$word" . "_" . "$counter"; if ( -e -d $name) { $counter++; next; } else { mkdir $name, 0755 or warn "Cannot make dir $name: $!"; $flag = '0'; } print "$name \n"; $name; } } $

I know that execution goes through the else clause because I have dozens of directories now. I believe that I have a correct form for making the $flag false in the same clause, but perl thinks that I do nothing to end this control. How come?

Also accepting any tips to make this more concise and readable. Thanks in advance,

Replies are listed 'Best First'.
Re: getting a while loop to terminate
by aaron_baugher (Curate) on Apr 15, 2012 at 05:33 UTC

    Your flag is working and breaking you out of your while loop just fine. However, you call your dirname() sub for every image found. If you put a line like the following at the beginning of your sub, you'll see what's happening:

    print "Entering sub dirname\n";

    For readability, I'd get rid of the flag variable altogether by putting my test in the while condition. I also find it awkward to have a break out of the first half of an if-else with the major work in the second half. (Not wrong, just awkward.) So I'd do something like this:

    sub dirname { print "Entering sub dirname\n"; my $word = "site"; my $counter = 1; my $name; do { $name = $word . "_" . $counter++; } while( -e $name); mkdir $name, 0755 or warn "Cannot make dir $name: $!"; print "$name \n"; }

    Aaron B.
    My Woefully Neglected Blog, where I occasionally mention Perl.

Re: getting a while loop to terminate (File::Temp)
by eyepopslikeamosquito (Archbishop) on Apr 15, 2012 at 07:01 UTC

    You're indentation is all over the place. If we fix that and analyze just your misbehaving subroutine:

    sub dirname { my $word = "site"; my $counter = 1; my $flag = 1; while ($flag) { my $name = "$word" . "_" . "$counter"; if ( -e -d $name) { $counter++; next; } else { mkdir $name, 0755 or warn "Cannot make dir $name: $!"; $flag = '0'; } print "$name \n"; $name; # "Useless use of private variable in void context" } }
    we see that Perl helpfully warns us that the $name; line above is useless. If we fix that by replacing the useless $name with return $name and change -e -d $name to -e $name and eliminate the now unnecessary $flag we will have a non-looping subroutine that kinda works. Note however that aaron_baugher has already shown us a much cleaner way to code your subroutine and his version is preferred to patching your original code.

    Having said that, this sort of code is tricky to get right, and race conditions and security exploits abound -- which is why Perl provides the File::Temp module. If you tell us your requirements for these "temporary" directories (and files?) we could offer more advice on this and I suspect the core File::Temp module could be employed to satisfy your requirements.

    Other random stylistic suggestions:

    • Don't call the subroutine with &dirname, use dirname() instead. See perlsub.
    • In my $filename = "$dir". "/image_". "$counter"; the quotes around $dir and $counter are pointless.
    • Probably better to think of a different name for your sub to avoid confusion with the Perl core File::Basename module which auto-exports a subroutine called dirname.

      Another reason to avoid dirname is because that function does more than return a name, it makes directories

      I choose dirpp because its sufficiently close to dirname but sufficiently inane :) The way I say it, its der-pup/der-poop/der-pee-pee. I suppose the PP could be interpreted as ++ (plus plus )

      I also suppose MakeIncDir / MakeSeqDir / NextDir could work as well.

Re: getting a while loop to terminate
by Anonymous Monk on Apr 15, 2012 at 04:15 UTC

     my $m = WWW::Mechanize->new;

    Replace with  WWW::Mechanize->new( qw/ autodie 1 /);

     getstore($url,$filename) or die "Can't download '$url': $@\n";

    Get rid of LWP::Simple , replace with  $m->mirror( $url, $filename ); autodie takes care of the dying

     if ( -e -d $name ) {

    While you can (since 5.9.1) chain/stack file-test operators in a case like this (it's the quivalent of  -d $name && -e _) , if its a directory, it already exists (otherwise how can it be a directory ), so if you're going to test to see if its a directory, just use -d

    Checking if a directory exists before creating the directory is a classic race condition --- some other program could make the directory after your program checks that it doesn't exist, but before it creates it -- this may or may not matter to your program

    Since creating a directory is an atomic operation (either succeed or fail), simply try to create the directory -- if it already exists, creation will fail

    Here is how I might write that

    sub Dirpp { use Errno qw/ EACCES /; # permission denied my $newdir = "site00"; my $made = 0; while ( not $made = mkdir $name, 0755 ) { die $! if $! == EACCES; $newdir++; } return $name if $made; # if we made it, return new name return; }

    This takes advantage of the magical string increment or auto-increment. auto-increment does overflow, so if that isn't desired, then I would write

    sub DirPP { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $counter = 1; my $name ; my $made = 0; while(1){ $name = sprintf '%s%3d', $word, $counter; last if not $made = mkdir $name, 0755 ; die $! if $! == EACCES; } return $name if $made; return; }

    Why  while(1) ? Because to me two sprintf looks clumsy

    sub DirPo { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $counter = 1; my $made = 0; my $name = sprintf '%s%3d', $word, $counter; ## ONE, PEE while ( not $made = mkdir $name, 0755 ) { die $! if $! == EACCES; $name = sprintf '%s%3d', $word, $counter; ## TWO, EEW } return $name if $made; return; }

    Also, storing one file per directory is bizzare :)

      To avoid duplicate checks during the run of a program, I would optimize by making $counter a state variable

      The modern perl way

      use feature qw/ state /; state $counter = 1;

      Or the old way, with a closure

      BEGIN { my $counter = 1; sub DirPP { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $name ; my $made = 0; while(1){ $name = sprintf '%s%3d', $word, $counter; last if not $made = mkdir $name, 0755 ; die $! if $! == EACCES; } return $name if $made; return; } }

        I'm really having a hard time putting responses where they need to be. Since my re-working of what aaron posted and what AM posted are catching in the same place, I hope that putting the responses here doesn't offend.

        I had two big problems in the original post that have now been corrected: single quotes around the zero for false and calling for a new DirPP while in the loop that downloads the images.

        Now I'm trying to integrate the slicker versions of DirPP and stumble on the same error despite which version I try:

        $ perl tg2.pl Unrecognized LWP::UserAgent options: autodie at tg2.pl line 7 Use of uninitialized value $dir in string at tg2.pl line 16. Use of uninitialized value $dir in string at tg2.pl line 16. ... Use of uninitialized value $dir in string at tg2.pl line 16. ^C $ cat tg3.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; use feature qw/ state /; my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new( qw/ autodie 1 /); $m->get( $domain); my $counter = 0; my @list = $m->images(); my $dir = &DirPP; for my $img (@list) { my $url = $img->url_abs(); $counter++; my $filename = "$dir". "/image_". "$counter"; #line 16 getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub DirPP { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $counter = 1; my $name ; my $made = 0; while(1){ $name = sprintf '%s%3d', $word, $counter; last if not $made = mkdir $name, 0755 ; die $! if $! == EACCES; } return $name if $made; return; } $

        From the look of the output, the subroutine is being called many times, but to my eye, I call it only once, and that is before I enter the loop that assigns a value to $dir . (scratches head) That was aaron's version, but I induce the same behavior with AM's version:

        $ perl tg2.pl Unrecognized LWP::UserAgent options: autodie at tg2.pl line 7 Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. Use of uninitialized value $dir in string at tg2.pl line 15. ^C $ cat tg2.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; use feature ':5.10'; my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new( qw/ autodie 1 /); $m->get( $domain); my @list = $m->images(); my $counter = 0; my $dir = &DirPP; for my $img (@list) { my $url = $img->url_abs(); $counter++; my $filename = "$dir". "/image_". "$counter"; getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub DirPP { use Errno qw/ EACCES /; # permission denied state $counter2 = 1; my $word = "site"; my $name ; my $made = 0; while(1){ $name = sprintf '%s%3d', $word, $counter2; last if not $made = mkdir $name, 0755 ; die $! if $! == EACCES; } return $name if $made; return; } $

        So, I'm stumped and out of guesses. That means I have no excuse to stay on the computer...off to the world of work..., happy tuesday,

Re: getting a while loop to terminate
by thezip (Vicar) on Apr 15, 2012 at 03:55 UTC

    It's because you have set $flag to '0', which is an uninterpolated string containing the character '0', a true value.

    Instead, you should set $flag to zero, (0), without the single quotes. This will evaluate to false and make your logic all happy.


    What can be asserted without proof can be dismissed without proof. - Christopher Hitchens, 1949-2011

        I guess I don't follow you here:

        $ perl -le " die 666 if q/0/ " $ perl -le " die 666 if $what = q/0/ " syntax error at -e line 1, near "if =" Execution of -e aborted due to compilation errors. $

      Yay! first taste of victory....

      $ perl tg1.pl site_13 $ cd site_13 $ ls image_1 image_2 image_3 ... $ cd .. $ cat tg1.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new; $m->get( $domain); my $counter = 0; my $dir = &dirname; my @list = $m->images(); for my $img (@list) { my $url = $img->url_abs(); $counter++; my $filename = "$dir". "/image_". "$counter"; getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub dirname { my $word = "site"; my $counter = 1; my $flag = 1; while ($flag) { my $name = "$word" . "_" . "$counter"; if ( -d $name) { $counter++; next; } else { mkdir $name, 0755 or warn "Cannot make dir $name: $!"; $flag = 0; } print "$name \n"; return $name; } } $

      I have a whole bunch of questions at this point, but don't want to ask them all. Let me ask this one, because it goes to readability: Is there an indent command that works well for perl? indent -i2 foo.pl was not a winner.

        You may be looking for Perl::Tidy which does an excellent job of pretty printing Perl and is highly configurable.

        True laziness is hard work

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others rifling through the Monastery: (1)
As of 2024-04-25 05:51 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found