7
\$\begingroup\$

I took a lot of pictures with my camera and I wanted to make a time-lapse out of them. The camera saved the pictures as picture1, picture2 ... picture956 etc. but the time-lapse software I'm using only accepts numbers of equal length like this: picture001, picture002, picture003 etc.

I thought Perl would be a good fit for this kind of problem so I gave it a shot. This is a shorter and translated version of the original code so if anything is unclear I can change it to the longer version.

use strict;
use warnings;
use diagnostics;

print "Give the path to the directory where the pictures are stored:\n";
my $filename = <STDIN>;

chomp $filename;
chdir $filename or die "Couldn't change the directory: $!\n";
my @files  = <*>;
#gives a list of all filehandles in the given directory

my $amount_of_digits = int( log($#files)/log(10) +1);
#how many digits should the new number have?
#Example: If there are 400 files -> 3 digits per file: 001, 002 etc

foreach my $file (@files){
    next if($file =~ /^\.$/);
    next if($file =~ /^\.\.$/);
    #skip the . and .. files
    print "$file\n";
    if ($file =~ /(\d+)/){
        my $amount_of_padded_zeroes = $amount_of_digits - length($1);
        if($amount_of_padded_zeroes > 0){
            my $new_number =  '0' x $amount_of_padded_zeroes . $1;     
            (my $new_name = $file) =~ s/$1/$new_number/;
            print "Changing name too: $new_name\n"; 
            rename ($file, $new_name) or die "Couldn't rename the file: $!";
        }
    }
}

print "Program completed. Press any key to continue.\n";
my $einde = <STDIN>;

The code works but I wanted to know if there is a better/cleaner/shorter/more Pearlesque way to do this. I'm learning Perl for uni so any feedback is welcome. I suspect this problem is so trivial in Perl that there might be readable one-liners that are able to replace all of this.

\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

As you suspect, this can be done in fewer lines, but it's not a one-liner.

use strict;
use warnings;
use diagnostics;

Always a good idea. Consider use warnings FATAL => "all" so that you don't miss any.


my $filename = <STDIN>;

A command-line argument or environment variable is the typical way to do this, in Perl and most other languages. Directory variables oughtn't be named $filename.

  my $dir = ( shift or $ENV{TIMELAPSE_DIRECTORY} or die "usage: $0 directory\n" );

chdir $filename or die "Couldn't change the directory: $!\n";
…
rename ($file, $new_name) or die "Couldn't rename the file: $!";

The fat-arrow => can replace a comma and improve readability in some cases. Dropping parens is another good readability boost, when done judiciously. Unless the line number where die occurred is interesting, suppress it by appending a newline to the message. And finally, it's a good habit to include in error messages the data that produced them, as in:

 rename $file => $new_name or die "Couldn't rename '$file' to '$new_name': $!\n";

my @files  = <*>;
#gives a list of all filehandles in the given directory

Filenames, not filehandles. It would be reasonable to filter this list to contain actual files, excluding directories:

my @files  = grep -f, <*>;

And maybe even by name:

my @files = grep { -f and /\.( png | jpe?g | tga )$/xi } <*>;

my $amount_of_digits = int( log($#files)/log(10) +1);

$#files is the largest index in that zero-based array. With 10 files, the last is $files[9] and the math returns 1 instead of the 2 we need. The size of the array is one bigger and retrieved as @files in scalar context (log() imposes scalar context for us, which is convenient).

width is a good name for this variable.

Avoid an uncaught exception by checking that @files is non-empty.

And the parens around int can be omitted.

die "nothing to do!\n" unless @files;
my $width = int log(@files)/log(10) + 1;

But this is still no good! log(1000)/log(10) is 3 in Perl and in real life. But int( log(1000)/log(10) ) is 2! This happens because floating-point math is imperfect. Luckily Perl will let us cheat by taking the length of a number that's been silently converted to a string:

my $width = length scalar @files;

foreach my $file (@files){

for is the idiomatic alternative to foreach.


    if ($file =~ /(\d+)/){
        my $amount_of_padded_zeroes = $amount_of_digits - length($1);
        if($amount_of_padded_zeroes > 0){
            my $new_number =  '0' x $amount_of_padded_zeroes . $1;     
            (my $new_name = $file) =~ s/$1/$new_number/;
            print "Changing name too: $new_name\n"; 

sprintf is the function to use for leading zeroes. It can go right in the s/// replacement by using the /execute modifier. If the replace fails, skip to next file.

             (my $new_name = $file) =~ s/(\d+)/ sprintf "%0${width}d" => $1 /e or next;

rename ($file, $new_name) or die "Couldn't rename the file: $!";

It's good practice to ensure you have something to do, and that you aren't overwriting an existing file here:

next if $new_name eq $file;
die "$new_name (from $file) already exists!\n" if -f $new_name;

print "Program completed. Press any key to continue.\n";
my $einde = <STDIN>;

I think you know how I feel about this. Should you decide to keep it, only the Enter key will actually proceed.

Putting it all together:

use strict;
use warnings FATAL => 'all';
use diagnostics;

my $dir = ( shift or $ENV{TIMELAPSE_DIRECTORY} or die "usage: $0 directory\n" );
chdir $dir or die "Couldn't change directory to $dir: $!\n";
my @files = grep { -f and /\.( png | jpe?g | tga )$/xi } <*>;
die "nothing to do!\n" unless @files;
my $width = length scalar @files;

for my  $file (@files) { 
    (my $new_name = $file) =~ s/(\d+)/ sprintf "%0${width}d" => $1 /e or next;
    next if $new_name eq $file;
    die "$new_name (from $file) already exists!\n" if -f $new_name;
    rename $file => $new_name or die "Couldn't rename '$file' to '$new_name': $!\n";
}
print "Program completed.\n"

Reader exercise: improve this program to number the files from 1 to n, even if the original numbers don't start at 1 or have gaps!

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Remove diagnostics in production. They take about a second to load and add no value once your program works. They just explain things after there were errors or warnings. \$\endgroup\$
    – simbabque
    Oct 8, 2019 at 8:37
  • \$\begingroup\$ Your diagnostics advice is perfectly reasonable; the time estimate seems high. For me, it took 95ms to load diagnostics (2012-vintage Core i3, in 1.6GHz powersave mode, with no reason for diagnostics.pm to be in disk cache). On a more recent Core i5 it takes about half that time, 50ms. \$\endgroup\$ Oct 8, 2019 at 12:30
  • \$\begingroup\$ you are right. I misremembered the time it takes. My advice was based on my experience here, but that is a few years ago, when hardware was less powerful. But indeed it was only 300ms. No idea where I had the second from. \$\endgroup\$
    – simbabque
    Oct 9, 2019 at 8:33
  • \$\begingroup\$ It's really hard to see where your code jumps. die and next shouldn't be hidden halfway into a statement. Add a line break before or die, or next, etc. \$\endgroup\$
    – ikegami
    Nov 7, 2019 at 6:27
  • \$\begingroup\$ those are exceptions. Noticing them is not required to understand the program flow, and they're not important enough to merit the beginning of a line. \$\endgroup\$ Nov 7, 2019 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.