Hi everyone
I'm new to Perl and find the learning process both exciting and challenging. I'd like some feedback on the following please:
#!/usr/bin/perl
use strict;
use warnings;
my $dir = "C:\\Tickets";
if (chdir "$dir") {
opendir (DIR, $dir);
my @fileList = readdir DIR;
foreach my $oldname (@fileList) {
next if -d $oldname;
my $newname = $oldname;
$newname =~ s/Closed/Open/;
rename $oldname, $newname;
}
}
else {
print "Error - Please check that $dir exists and is accessible.\n";
}
closedir (DIR);
It renames all files in $dir containing the word "Closed" in the filename to "Open". Is there a shorter / better way of accomplishing this task? Any advice or suggestions are welcome.
You can reduce the amount of work by restricting the file list to those which actually contain "Closed". Do that by replacing opendir . . . readdir . . . closedir with [glob]. You can also shorten your loop code a little by taking advantage of $_ and its properties.
#!/usr/bin/perl
use strict;
use warnings;
my $dir = 'C:\Tickets';
chdir $dir
or die "Error - Please check that $dir exists and is accessible.\n";
my @fileList = glob '*Closed*';
foreach (@fileList) {
next if -d;
my $oldname = $_;
s/Closed/Open/;
rename $oldname, $_;
}
I also removed the big if..else by just [die]ing with the error message if [chdir] fails. That would avoid [closedir] on a bad handle in your code. Moving [closedir] inside the affirmative branch would do that, too.
Update: You can remove the dependence on [chdir] entirely by including $dir in the [glob] pattern. With the error handling for [rename] added, the new version looks like this,
#!/usr/bin/perl
use strict;
use warnings;
my $dir = 'C:\Tickets\';
my @fileList = glob "${dir}*Closed*";
foreach (@fileList) {
next if -d;
my $oldname = $_;
s/Closed/Open/;
rename $oldname, $_ or
$_ = $oldname,
warn $_, ' not renamed: ', $!;
}
After Compline,
Zaxo
Thanks Zaxo.
I'm taking notes :-)
rename $oldname, $_ or die "Can't rename $oldname to $_: $!";
[Joost]++ on checking. That was careless of me. In that case, though, I'd probably just warn so the loop can keep on trying. While I'm at it, why not change $_ back so I still have an accurate file list after I'm done?
rename $oldname, $_ or
$_ = $oldname,
warn "Can't rename $oldname to $_: $!";
After Compline,
Zaxo
Thanks Joost
I've been told by some developers to try and not use die in my code if possible. Reasons being that it is not "pretty" and it might complicate debugging sometimes (if not used properly).
I think one good reason for using it though, like Zaxo stated, is to avoid further bad handles.
There will be times where you may not want the entire program execution to die if you encounter a problem. Instead, it's always a good idea to get into the habit of handling problems gracefully. For example, always test to see if an open or rename fails and then decide what to do at that point. Issuing a die might be an appropriate reaction. Or, depending on the program in question, you might want to log that error information somewhere, retry the action that failed or perform some other function instead of issuing a die.
You really have to decide on a case by case basis as to what makes the most sense. The main thing to keep in mind is to always check return values and be prepared to handle error conditions.
use File::Rename qw(rename);
my @list = `dir`;
rename @list, sub { s/Closed/Open/ }, 1;
I dodidn't think my @list = `dir`; does what you think it does. Backticks will put the entire listing into a single string, $list[0]. File::Rename's overridden rename will fail since the listing will never match a file name (even with only one file - newline tacked on).
Here, too, glob would be preferable for populating @list.
Update: Oops. Major thinko. I'll stand by preferring glob to shelling out.
After Compline,
Zaxo