in reply to Re^4: getting a while loop to terminate
in thread getting a while loop to terminate

Thx aaron and AM, I've got versions working now that are pretty close to what you prescribed:

$ perl tg4.pl site_1 site_2 site_3 site_4 site_5 site_6 site_7 site_8 site_9 site_10 site_11 site_12 site_13 site_14 site_14 $ cd site_14 $ ls image_1 image_2 image_3 image_4 image_5 image_6 image_7 +image_8 ... $ cd .. $ cat tg4.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; use feature ':5.10'; # get information about images my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new(); $m->get( $domain); my @list = $m->images(); # create new folder and download images to it. my $counter = 0; my $dir = &DirPP; for my $img (@list) { my $url = $img->url_abs(); $counter++; my $filename = "$dir". "/image_". "$counter"; #line 15 getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub DirPP { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $counter2 = 1; my $name; my $made = 0; while(1){ $name = "$word" . "_" . "$counter2"; $counter2++; print "$name \n"; last if $made = mkdir $name, 0755 ; die $! if $! == EACCES; } print "$name \n"; return $name if $made; return; }

Again, I'm looking for tips on style or cautions for robustness. Since one can now demonstrate two ways to do this task, reasonable people could disagree on the final touches, and the discussion might help me to understand these routines better. Here's the one that uses a state variable. Do I use it correctly?

$ perl tg5.pl site_1 site_2 site_3 site_4 site_5 site_6 site_7 site_8 site_9 site_10 site_11 site_12 site_13 site_14 site_15 site_16 site_16 $ cat tg5.pl #!/usr/bin/perl -w use strict; use WWW::Mechanize; use LWP::Simple; use feature ':5.10'; use feature qw/ state /; state $counter3 = 1; # get information about images my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new(); $m->get( $domain); my @list = $m->images(); # create new folder and download images to it. my $counter = 0; my $dir = &DirPP; for my $img (@list) { my $url = $img->url_abs(); $counter++; my $filename = "$dir". "/image_". "$counter"; #line 15 getstore($url,$filename) or die "Can't download '$url': $@\n"; } sub DirPP { use Errno qw/ EACCES /; # permission denied my $word = "site"; my $name; my $made = 0; while(1){ $name = "$word" . "_" . "$counter3"; $counter3++; print "$name \n"; last if $made = mkdir $name, 0755 ; die $! if $! == EACCES; } print "$name \n"; return $name if $made; return; }

Replies are listed 'Best First'.
Re^6: getting a while loop to terminate
by aaron_baugher (Curate) on Apr 18, 2012 at 07:16 UTC

    Well, I have yet to use the state feature myself, so I may not be the best judge of that, but in this case, it doesn't gain you anything. If you just instantiate $counter3 normally with my, it will be available to subroutines within the file already, without needing state. It won't hurt anything; it just has no particular effect here.

    With regard to your logic for finding a new directory name and creating it, I would be concerned about a different kind of race condition. If the mkdir fails because of permissions, your next line deals with that with die. Otherwise it keeps incrementing the counter until it finds a name that can be created. But what if the mkdir fails for some other reason, like EROFS or ENOSPC? Your loop will continue forever, incrementing and trying to mkdir until you kill the program. That's why my version did the mkdir after my do/while loop, and only used the loop to find an available name. As others pointed out, you still need an error check on the mkdir, just in case some other process creates the directory in that split second between your loop selecting the name and mkdir making it, but that's simpler than trying to specifically catch all the possible errors that mkdir might throw.

    The double return is awkward, partly because it should never be possible to trigger the second one. Your while loop can only be exited if $made is true, in which case the first return will be executed, or by die, in which case neither return will be reached. The second return is thus a superfluous line that cannot be reached and looks like a possible typo or something.

    One last stylistic note: You don't need double quotes to interpolate a single variable, so these are equivalent:

    my $filename = "$dir". "/image_". "$counter"; my $filename = $dir. "/image_". $counter; my $filename = "$dir/image_$counter";

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

      After giving it a bit more thought, I realized that the only "acceptable" error from mkdir (the only one you want to continue looping after) is EEXIST (File exists). So maybe it makes more sense to loop on that error. That eliminates the need for any flags, or any extra error checking to avoid a race condition:

      $my $counter = 1; use Errno qw[ EEXIST ]; sub mk_new_dir { while(1){ my $name = $word . '_' . $counter++; if( mkdir $name, 0755 ){ return $name; # success, return new dir name } else { next if $!{EEXIST}; # mkdir failed because the file exists; loop + and try next name die $!; # it failed for some other reason; bail out! } } }

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

        I thought I posted the final version of this. I've been reading as I come here and noticed that it's a possibility that if your thread dithers on too long, someone might just cut it off. I don't know whether that happened, or I just forgot to click the create button, but I don't think a thread has finished until one has a version with a bit of polish:

        $ perl tg6.pl downloaded 83 images from http://www.yahoo.com to folder site_4 $ cat tg6.pl #!/usr/bin/perl -w # creates a new directory and downloads images from url to it # perlmonks node 965537; thx aaron and A.M. use strict; use feature ':5.10'; use WWW::Mechanize; use LWP::Simple; use Errno qw[ EEXIST ]; # get information about images my $domain = 'http://www.yahoo.com'; my $m = WWW::Mechanize->new(); $m->get( $domain); my @list = $m->images(); # create new folder and download images to it. my $counter = 0; my $dir = &mk_new_dir; 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"; } # output print "downloaded ", $counter, " images from ", $domain, "\n"; print "to folder ", $dir, "\n"; sub mk_new_dir { my $counter2 = 1; while(1){ my $word = "site"; my $name = $word . '_' . $counter2++; if( mkdir $name, 0755 ){ return $name; # success, return new dir name } else { next if $!{EEXIST}; # mkdir failed because the file exists die $!; # it failed for some other reason; bail out! } } $

        I think aaron's integrating the 'mkdir $name, 0755' as the truth condition of the sub is great, as well as the post-increment of $counter2 to drive the thing forward.

        }