in reply to Rename All Files In a Directory

As a general rule, you're missing two important lines: 
use strict;
use warnings;

[download]
Said this, I would use glob, rather than opendir & friends. In any case your code doesn't seem too bad, for you take care of specifying the full path to your files. But notice that 
$dir =~ m|/$|;  # Do you get the point?!?

[download]

Update: I'm an idiot - it had been chopped, and now I see why you did do it in the first place. Still, it makes for clumsy logic and code, IMHO. PS: $dir does not need to be chopped, and if it did, then it would have been better chomped instead. Also, consider using a more effective indenting style.

Update: ditto as the (update) above!

PS2: now that I look at your script more closely, I feel like pointing out too that since it's not Perl6 yet (thus we do not have zip), your logic is somewhat awkward: why not doing it all in one iteration rather than in two?!? That is what most people would do even if we already had better means to iterate over two arrays in parallel - but maybe even then an AoA would be preferable. TMTOWTDI, you know...

Replies are listed 'Best First'.
Re^2: Rename All Files In a Directory
by holli (Abbot) on Jul 01, 2005 at 09:45 UTC
    A little walkthrough 
    $dir="/home/yehuda/img/jpg/";
chop $dir;

    [download]
    Here you hardcode a string and immedeatly chop off the last char. I cannot make any sense of that. 
    while (defined ($img=readdir DH)){
$img=~m/^(.*)\.(.*)\.(.*)/g;
$newname="$1.$3";

    [download]
    Here you do not check if the regex matches. If there is file that does not meet your assumptions it will horribly fail. 
    push @names,$img;
push @newnames,$newname;
}

    [download]
    I'm not sure why you don't rename the files immedeatly, but ok. i can live with that. Even if you really have to store the filenames somewhere i would use a hash for that. old name as key, new name as value. 
    close DH;

    [download]
    close() closes a filehandle, not a dirhandle. use closedir(); 
    foreach $name(@names){
$i=$i+1 ;
$newname_=$newnames[$i-1];

    [download]
    Now that is the fun part. You add 1 to $i just to substract the 1 again when you use $i. Better to do the addition after the usage and leave out the substraction. 
    $path="$dir/$name";
$target="$dir/$newname_";

    [download]
    Two unneccessary variables. 
    chomp($target,$path);

    [download]
    Why chomp? there is no newline insight. 
    rename ($path,$target) or die "Could not rename:$!";
print "  $path   is now $target \n";

    [download]
    I'd prefer a warning here instead of sudden death.

    After all I doubt that you got this code from the cookbook. And if so, throw it into the trashcan. Slightly improved: 
    $dir="/home/yehuda/img/jpg";

opendir(DH,$dir)or die "FOO BAR! $!\n";

while ( readdir DH)
{
    if ( -f "$dir/$_" && $m/^(.*)\.(.*)\.(.*)/ )
    {
        warn "cannot rename $dir/$_\n"
            unless rename "$dir/$_", "$dir/$1.$3";
    }
}
closedir DH;

    [download]


    holli, /regexed monk/
[reply]
[d/l]
[select]
      $dir="/home/yehuda/img/jpg/";
chop $dir;

      [download]
      Here you hardcode a string and immedeatly chop off the last char. I cannot make any sense of that.
      It does have some minimal amount of sense, if seen in the awkward context of all the rest of the code. Probably the trailing / conveys him the psychological feeling of having to do with a directory, but then later he does $dir/$somethingelse. Not to justify him - just trying to understand...

      Incidetally, why did you post your reply to the OP as a reply to my own reply?!?

[reply]
[d/l]
[select]
        Because in your OP, before you updated it, there was something along the lines of "your code does not look so bad".


        holli, /regexed monk/
[reply]

In Section Seekers of Perl Wisdom