Using a variable from outer scope in a sub called in foreach

Question

I am using cygwin. What this script does is load the iphone pics that I have loaded into a directory on the desktop. It opens it up in image viewer, to let me take a look at the picture.

system("cygstart $dirname/$oldfile") ;

Then it gives me the option to rename the picture. It is throwing errors though, and not renaming the picture.

Use of uninitialized value $oldfile in concatenation (.) or string at ./rename_image.pl line 29, <STDIN> line 6.

oldfile is a global varaible, the functions should see the variable.

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

my $oldfile;
my $new_name;
my $dirname = "/cygdrive/c/Users/walt/Desktop/iphonepics/bunk_box/";
opendir(DIR, $dirname) or die "Cannot open dir: $!";

my @files = readdir(DIR);
foreach $oldfile (@files) {
        system("cygstart $dirname/$oldfile") ;
        print "Do you want to rename $oldfile ? ";
        my $input = <STDIN> ;
        chomp $input ;
        if($input =~ m/^[y]$/i) {
                rename_file() ;
        } else {
        my $doo = 1 ;
        }
}

sub rename_file {
        use File::Copy qw(move) ;
        print "New name:\n" ;
        my $new_name = <STDIN> ;
        chomp $new_name ;
        move "$dirname/$oldfile", "$dirname/$new_name";
        return ;
}

Are we seeing 100% of the code? There is no possible way for rename_file to have been executed except from within the foreach loop? — DavidO
– DavidO, Commented Sep 25, 2017 at 6:12
This is unrelated to the warning you are seeing, but have you noticed that your paths will look like this? /cygdrive/c/Users/walt/Desktop/iphonepics/bunk_box//foobar (In other words, you're concatenating a string that ends with / to the filename, using another / in your concatenation. — DavidO
– DavidO, Commented Sep 25, 2017 at 6:15
Yes this is the code, and no, it does not rename any files. It so far does not work as planned. Also, no I did not notice the extra slash in the $dirname variable. what is the proper, accepted way to delineate an absolute path as a variable - ending in a slash or open (with the slash being assumed) — capser
– capser, Commented Sep 25, 2017 at 17:31
All I want to do is rename some pictures - the cygstart is just do I can see what picture is loaded into the array, the visual helps me figure uot a name for it. Most of the Pictures are named IMG_* by default. — capser
– capser, Commented Sep 25, 2017 at 17:32
Doubling a slash should have absolutely no effect (but I don't use Cygwin much, please test). Whether you include it in the directory or when you compose a path is a matter of choice of the convention you want: either $dir = '.../'; and $file = $dir . $name; OR $dir = '...' (no slash) and $file = "$dir/$name". But you might as well use both, to be on the safe side. — zdim
– zdim, Commented Sep 25, 2017 at 17:38

zdim · Accepted Answer · 2019-10-24 05:22:27Z

1

The foreach $oldfile (@files) does not assign values to the my $oldfile declared at the top. That one is a lexical variable, not global, and in such a case a new lexical $oldfile is created for the loop's scope. This is a particular property of foreach alone. See this post for details.

Since the $oldfile in foreach is a lexical in a dynamic scope it is invisible to the sub. But the sub sees my $oldfile declared for the static scope of the file. From Private Variables via my()

... lexical variables declared with my are totally hidden from the outside world, including any called subroutines. This is true if it's the same subroutine called from itself or elsewhere--every call gets its own copy.

This doesn't mean that a my variable declared in a statically enclosing lexical scope would be invisible. Only dynamic scopes are cut off. For example, ...

That $oldfile at the top, seen by the sub, stays as it was before the loop, uninitialized.

Altogether, a simple "fix" to get the expected behavior is to make $oldfile on top a global variable, so to replace my $oldfile with our $oldfile.^†

However, why rely on global variables? Why not pass to functions what they need

foreach my $oldfile (@files) {
    ...
    if (...) {
        rename_file($dirname, $oldfile);
    ...
}

sub rename_file {
    my ($dirname, $oldfile) = @_;
    ...
}

You are nicely using strict already and declaring everything else. Then you don't need, and should not have, the file-wide lexical my $oldfile; declared on top.

When you declare a variable in a sub, it is shielded from variables with the same name outside the sub's scope. So here you know what $oldfile is – it is exactly what you passed to the function.

That way your sub also has a well defined interface and does not depend on arbitrary values in the surrounding code. This is crucial for clear delineation of scope and for modularity.

More generally, using a global variable while strict is in place and other things declared is like planting a mine for the maintenance programmer, or for yourself the proverbial six months later.

I suggest to also read Coping with Scoping by Mark-Jason Dominus.

^† I don't recommend this unless there is a very specific and solid reason for a global-like our variable.

edited Oct 24, 2019 at 5:22

answered Sep 25, 2017 at 6:12

zdim

67.2k5 gold badges59 silver badges87 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

DavidO Over a year ago

This is all sound advice, but if the code we're seeing is 100% of the code the OP is using, the warning should not be possible, as it would mean that readdir populated @files with an undef entry, and furthermore, that the user of the script managed to still say "Y" to the question of whether to move the file, when the filename didn't show up in the screen output. Given it's unlikely that we're seeing the code that produced this warning, it's hard to come to the conclusion that this answer actually solves the OP's question.

zdim Over a year ago

@DavidO I added the explanation for the warning, thanks for prompting it. As for the "impossible" -- it isn't, it's just how it has to be. Try it, give some value to my $oldfile when it is declared.

capser Over a year ago

Yes - this is the code in its entirety - all it does is open up the photo in photoviewer. I look at the photo, which new photos taken with my iphone typically are called IMG_1234.jpg. If I like it I hit yes and rename the photo. Anything left IMG_* will be removed from the directory.

zdim Over a year ago

@capser Thank you for confirming it -- I thought so and answered your question about the particular error. Did you mean to respond to DavidO? (If so tag your comment with AT-DavidO)

zdim Over a year ago

@capser Yes, in my opinion this is a good question, that is important as a good lesson. I hope that I explained it well enough, if not please let me know what is unclear in my answer and I'll rephrase things. Thank you for attribution.

|

Collectives™ on Stack Overflow

Using a variable from outer scope in a sub called in foreach

1 Answer 1

7 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

7 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related