4
\$\begingroup\$

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;

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

\$\endgroup\$
1
  • \$\begingroup\$ Also note that directory 0 cannot be specified as the argument. \$\endgroup\$
    – choroba
    Apr 21, 2014 at 22:36

1 Answer 1

3
\$\begingroup\$

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
\$\endgroup\$

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.