Sort files into folders by filename suffix

Question

This is my first ever real 100% self-made Perl script. I'd like to know how I can improve it.

#!/usr/bin/env perl
#this script takes a directory and sorts the files into folders by file ending

use strict;
use warnings;
use File::Copy;

sub sort_files {
    my $dir = shift || '.';     #cwd if no directory given
    $dir =~ s/\/$//;            #remove trailing /

    opendir(DIR, $dir) or die $!;
    my @files = grep {
        !/^\./      #does not start with .
        && -f "$dir/$_" #is file
        } readdir DIR;

    my $len = @files;
    print "sorting $len files\n";
    foreach my $file (@files){
        my ($ext) = $file =~ /([^.]*$)/;#grabs file extension

        my $dest = "$dir/$ext/";
        if (not -d "dest"){     #create folder if it does not exist
            mkdir $dest;
            print "\tcreated directory $dest\n";
        }
        move "$dir/$file", $dest or die "$!";
        print "\t$file moved to $dest\n"
    }
}

sort_files @ARGV;
exit 0;

I just noticed that this script runs into problems if there is no file ending specified. Elegant solutions are appreciated (checking the file for !/\./ would work, but that's not elegant in my book).

Answer 1

I've added a few comments in your source,

#!/usr/bin/env perl

use strict;
use warnings;
use autodie; # automatic die/checks for http://search.cpan.org/~pjf/autodie/lib/autodie.pm#CATEGORIES
use File::Copy;

sub sort_files {
    # my $dir = shift || '.';
    my $dir = shift // '.'; # defined short circuit to allow '0' as argument
    $dir =~ s/\/$//;

    # opendir(DIR, $dir) or die $!;
    opendir(my $DIR, $dir);  # use lexical variable instead of glob
    my @files = grep {
        !/^\./      # not needed if you only want to skip '.' and '..'
        && -f "$dir/$_" 
        } readdir $DIR;

    my $len = @files;
    print "sorting $len files\n";

    foreach my $file (@files){
        my ($ext) = $file =~ /([^.]*$)/;

           #
           # what do you want to do when $ext is undefined?
           # my ($ext) = $file =~ /([^.]*$)/ or next; # to skip such files

        my $dest = "$dir/$ext/";
        # if (not -d "dest"){  # typo?
        if (not -d $dest){
            mkdir $dest;       # added autodie to check on mkdir
            print "\tcreated directory $dest\n";
        }
        move("$dir/$file", $dest) or die $!;   # autodie doesn't handle File::Copy
        print "\t$file moved to $dest\n"
    }
}

sort_files(@ARGV); # parentheses around arguments for non core functions are good idea https://eval.in/139428 vs https://eval.in/139430
# exit 0; # not needed

Answer 2

Overview

I agree with the previous answer on all points, but I have some additional suggestions.

There is a lot to like with your existing code:

• The shebang line has proven to be a solid portable choice
• Used strict and warnings
• Leveraged others' code with the use module line
• Good code layout with consistent indentation
• Meaningful names for variables

Here are some more suggestions.

Documentation

This is a good comment:

#this script takes a directory and sorts the files into folders by file ending

But, consider using plain old documentation (POD) and get manpage-like help with perldoc.

Fatal

My preference is to use a very strict version of warnings:

use warnings FATAL => 'all';

In my experience, the warnings have always pointed to a bug in my code. The issue is that, in some common usage scenarios, it is too easy to miss the warning messages unless you are looking for them. They can be hard to spot even if your code generates a small amount of output, not to mention anything that scrolls off the screen. This option will kill your program dead so that there is no way to miss the warnings.

Modules

It is best to import only what is needed to avoid namespace pollution. Change:

use File::Copy;

to:

use File::Copy qw(move);

Comments

#cwd if no directory given

The advantage of comments is that you can be as detailed as you want. It would be better to spell out "cwd" as "current working directory".

Regex

Instead of escaping the slash:

$dir =~ s/\/$//;

I like to use alternate delimiters ({}{}):

$dir =~ s{/$}{};

Simpler

You can eliminate a vaguely-named variable ($len):

my $len = @files;
print "sorting $len files\n";

using something like:

printf "sorting files: %0d\n", scalar @files;

Less typing

for can be used instead of foreach since they are synonyms.

Layout

I prefer to locate subs at the end of the code, after the executable code:

use File::Copy;

sort_files(@ARGV);

sub sort_files {

This means that you must use parens.