Renaming a file in a sub using variable from outer

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 ;
}

The foreach $oldfile (@files) does not assign values to the $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 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, that name is shielded from the same name outside of 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.

^† Or, one can drop my and drop strict. I do not recommend any of this unless there is a very specific and solid reason to have a global-like our $oldfile. See our.