HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Perl file sorting script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptsortingfileperl

Problem

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)

Solution

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

Code Snippets

#!/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

Context

StackExchange Code Review Q#47833, answer score: 3

Revisions (0)

No revisions yet.