1
\$\begingroup\$

I'm taking a scripting language course with no prior scripting language or Linux experience, though I've taken classes in VB, Java, and C++. Our first assignment is to write a script that will traverse the directory tree of the location the script is run from and rename the files according to some predetermined rules. In this case, directories are to have spaces, \", and any .hxx or .cxx extensions removed. Files get the same treatment except their extensions are changed to .h or .cpp respectively. Also, the script isn't supposed to actually change the file names, just produce a file with the commands to do so, mine are bash commands in the form of mv 'old file name' 'old_file_name'.

#!/usr/bin/perl -w

use Cwd;
use File::Find;
use strict;

my $file = "output.txt"; 
my $dir = getcwd;

# Creates file to print output to, Die with error message if we can't open it
open my $fh, '>>', $file 
    or die "$file: $!\n";

# traverse the directory tree
find(\&process_files, $dir);

# creates a file of bash rename scripts
sub process_files
{
    my $s1 = $_;    
    if (-f) 
    {
        $s1 =~ s/\s/_/g;
        $s1 =~ s/\"//g;
        $s1 =~ s/\.cxx/\.cpp/g;
        $s1 =~ s/\.hxx/\.h/g;
        print $fh "mv '" , $_ , "' " ,  $s1 , "\n";
        print "mv '" , $_ , "' " ,  $s1 , "\n";
    }
    if (-d) 
    {
        $s1 =~ s/\s/_/g;
        $s1 =~ s/\"//g;
        $s1 =~ s/\.(cxx|hxx)//g;
        print $fh "mv '" , $_ , "' " ,  $s1 , "\n";
        print "mv '" , $_ , "' " ,  $s1 , "\n";
    }
}
close $fh;
\$\endgroup\$
  • \$\begingroup\$ Not what you were asking, but be aware of the linux command rename (see here, or here for windows) which allows you to rename files using perl regexes. \$\endgroup\$ – Luke H Apr 27 '13 at 11:54
3
\$\begingroup\$

This is a great script you have (special thanks for using File::Find)!

There are a few, mostly stylistic issues I have with your code:

use warnings

I see you have the -w flag on the shebang line, but use warnings is more flexible. It has lexical scope, so you can deactivate warnings in a scope by no warnings, which sometimes is useful. In any way, it is a good habit to start.

The Unix Way: Writing Pluggable Programs

Much of the power of Unix comes from the little programs you can pipe together to transform data (see Unix Philosophy, esp “Write Programs that work together”). They all have in common that they accept input on STDIN, and (mostly) print to STDOUT. The user can then take that output and display it, write it to a file, or pass it to another program.

However, you print to both a file (in append mode! tell me a scenario where that is useful, and cat can't help), and STDOUT. The latter is enough. If the user wants the output on the screen, he does

$ /path/to/script

If he wants it in a file

$ /path/to/script > output.sh

If he wants both, the tee command is useful:

$ /path/to/script | tee output.sh # option -a for append mode possible

Your script basically duplicates tee functionality, this is unneccessary.

If you want the user to see certain output, regardless of output redirection, print to STDERR (this is what warn and die do).

Your script starts searching in the current working directory. This is a good default, but giving a start dir as command line option would be really sweet:

$ /path/to/script ~/look/.here

All we need for that is

my $dir = @ARGV ? shift() : getcwd();

Duplicate code? Consider refactoring.

I would write your process_files like

sub process_files {
    my $name = $_;
    # normalize name
    $name =~ s/\s/_/g;
    $name =~ s/"//g;
    # changes depending on type
    if (-f) {
        # normalize C++ extensions
        $name =~ s/\.cxx\z/.cpp/;
        $name =~ s/\.hxx\z/.h/;
    } elsif (-d) {
        # remove C++ extensions
        $name =~ s/\.[ch]xx\z//;
    } else {
      return; # unknown type? Ignore, leave sub
    }
    return if $name eq $_; # don't print mv command if there are no changes
    # common: print mv command
    print "mv -T '$_' $name\n";
}

It is a bit silly to refactor such little code, but I think it better conveys what we are doing. E.g. the name normalization is common. Inside the substitutions, I removed unneccessary escaping. The second half of a substitution has the semantics of a double-quoted string, so the period doesn't have to be escaped. As a personal preference, I tend to use \z (end of string) rather than $, (end of… whatever, depends on regex flags).

While you can pass multiple arguments to print, which will be concatenated automatically, you could also use the wonderful string interpolation Perl has—I find the above command more readable this way. Esp. when writing code that writes other code, clarity is a security feature.

Close

When a Perl process exits, remaining open filehandles are automatically closed. Lexical filehandles (as you are using), are closed as soon as their reference count drops to zero (i.e. when they go out of scope). So manual closing is generally useless except for following two cases:

  • You're using exotic buffering, and want to flush the buffer before closing
  • Your filehandle is actually connected to another process. The return value of close will indicate if the attached processed exited abnormaly.

… of which none apply here.

“Funny edge cases to destroy your program” or: Non-Stylistic Problems

When we are already talking about security: If I were paranoid, I'd worry about filenames that contain single quotes, and other fun like semicolons, or ';rm -rf * ;, which is a valid filename for Linux. To mitigate these issues, one could

use String::ShellQuote;
my $secure_orig_name = shell_quote $_;
my $secure_new_name  = shell_quote $name;
print "mv -T $secure_orig_name $secure_new_name\n";

but one could just as well assume that C++ source directories only contain sane filenames.

The -T flag for mv uses the second argument as the target name. Otherwise, it will check if there is a directory with that name, and move the file there. E.g.

# ./foo
# ./bar/
$ mv foo bar
# ./bar/
# ./bar/foo

but

# ./foo
# ./bar/
$ mv -T foo bar
mv: cannot overwrite directory `bar' with non-directory

which I'd rather prefer.

But it still isn't perfect. What if we rename a directory, but also some of its contents? Then the resulting mv command will try to rename a file whose path doesn't exists any more. The solution is to use the finddepth function instead of find, to process the contents of a directory before the directory itself is passed to your callback.

But it still won't work: File::Find will chdir into the currently inspected directory, so when we would run the mv commands, we'd be in the wrong working directory. But it's easy to sprinkle the code with cds.

use Cwd qw/abs_path/
my $dir = abs_path($dir);
my $cwd = $dir;
...;
sub process_files {
  # Inject cd wherever neccessary
  my $abs_path = abs_path($File::Find::dir);
  if ($cwd ne $abs_path) {
    $cwd = $abs_path;
    my $secure_cwd = shell_quote $abs_path;
    print "cd $secure_cwd\n";
  }
  ...;
}
\$\endgroup\$
3
\$\begingroup\$

It seems correct to me except these lines:

$s1 =~ s/\.cxx/\.cpp/g;
$s1 =~ s/\.hxx/\.h/g;
$s1 =~ s/\.(cxx|hxx)//g;

There're no needs to do the substitution g lobal and also you need to anchor the regex at the end:

$s1 =~ s/\.cxx$/.cpp/;
$s1 =~ s/\.hxx$/.h/;
$s1 =~ s/\.(cxx|hxx)$//;
\$\endgroup\$
1
\$\begingroup\$

There is a book called Perl Best Practices by Damian Conway that suggests some things to change:

Always use parentheses with functions (except builtins), even when they take no parameters:

my $dir = getcwd();

While / may be the historical standard, things like {} with natural matches are often better:

    $name =~ s{\s}{_}g;
    $name =~ s{\"}{}g;
    $name =~ s{[.]cxx\z}{.cpp};
    $name =~ s{[.]hxx\z}{.h};
    $name =~ s{[.](?:cxx|hxx)\z}{};
    $name =~ s{[.][ch]xx\z}{};

This also helps with Leaning Toothpick Syndrome, e.g. s/\s/// -- where it can get confusing as to what is a delimiter, what is an escape character, and what is part of the pattern. Contrast with s{\s}{/}.

Use non-capturing parentheses (?:) rather than using () when just grouping. Of course in this case, you only need to alternate a single character. A character class is better for that than grouped alternation. Thus [ch]xx is better than (?:cxx|hxx)

If matching a single character, a character class like [.] is often more readable than an escaped character like \.

I also followed M42's suggestions but used the more specific \z (end of string) rather than the $ (end of line).

Always use a FILEHANDLE with print statements. This avoids the accidental consumption of a variable as a file handle. E.g.

print STDOUT qq{mv -T '$original' $name\n};

Obviously assumes that you assign $_ to $original and rename $s1 to $name. Assigning $_ before using it is one of Conway's things. It leads to more readable code if you pick a better name than $_ for the new variable.

Use the more flexible qq{} over "", etc.

Note: I don't take Conway as gospel. He likes half-cuddled else clauses and I prefer mine fully cuddled. And I dropped his mandatory regex modifiers msx (at least most of the time). But some of his ideas make sense on their own. I probably forgot some that would apply. It's been a while since I read the book. Anyway, following Conway will usually produce more readable and maintainable Perl code than just winging it. Even just reading Conway and thinking about what you want to incorporate can help. Perl has so many possible bad practices that it is easy to get committed to them before you realize their downfalls.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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