4
\$\begingroup\$

I have some subroutines which I commonly copy & paste into much of my work. I recently posted this on StackOverflow and user simbabque suggested that I post my code here for reviews and comments:

#!/usr/bin/env perl

use strict; use warnings; use Cwd;
my $TOP_DIRECTORY = getcwd();
local $SIG{__WARN__} = sub {#kill the program if there are any warnings
    my $message = shift;
    my $fail_filename = "$TOP_DIRECTORY/$0.FAIL";
    open my $fh, '>', $fail_filename or die "Can't write $fail_filename: $!";
    printf $fh ("$message @ %s\n", getcwd());
    close $fh;
    die "$message\n";
};#http://perlmaven.com/how-to-capture-and-save-warnings-in-perl

sub execute {
    my $command = shift;
    print "Executing Command: $command\n";
    if (system($command) != 0) {
        my $fail_filename = "$TOP_DIRECTORY/$0.fail";
        open my $fh, '>', $fail_filename or die "Can't write $fail_filename: $!";
        print $fh "$command failed.\n";
        close $fh;
        print "$command failed.\n";
        die;
    }
}

sub run_parallel {
    my $command_array_reference = shift;
    unless ((ref $command_array_reference) =~ m/ARRAY/) {
        print "run_parallel requires an array reference as input.\n";
        die;
    }
    use Parallel::ForkManager;
    my $manager = new Parallel::ForkManager(4);
    my $START_DIRECTORY = getcwd();
    foreach my $command (0..scalar @{ $command_array_reference }-1) {
        $manager->start and next;
        my ($dir, $cmd) = @{ $command_array_reference->[$command] };
        chdir $dir or die "Can't chdir to $dir: $!";
        execute( $cmd );
        chdir $START_DIRECTORY or die "Can't chdir to $START_DIRECTORY: $!";
        $manager->finish;
    }
    $manager->wait_all_children;#necessary after all lists
}

my @commands;

$commands[0][0] = 'tmp1';#directory
$commands[0][1] = 'echo "ave Maria" > aveMaria.txt';#command

$commands[1][0] = 'tmp2';#directory
$commands[1][1] = 'echo "IN HOC SIGNO VINCES" > xp.txt';#command

run_parallel(\@commands);
\$\endgroup\$
  • \$\begingroup\$ @Sam I like how you smuggled a share link in there. ;) \$\endgroup\$ – simbabque Dec 4 '17 at 22:48
5
\$\begingroup\$

I'll do a first pass where I point out syntax and Perl conventions, mostly line by line. I will then talk about alternative strategies to make it more efficient and maintainable.

Syntax

use strict; use warnings; use Cwd;

You have use strict and use warnings. Well done!

You would typically have one use or require statement per line. That way, it's easier to read them.

You import getcwd from Cwd. It's a good practice to never have default imports, but to either specifically import the stuff you want, or not to import any names at all. This way, you don't pollute your namespace (even though you are in main implicitly), and later you know where functions in your code came from.

use strict;
use warnings;
use Cwd 'getcwd';

open my $fh, '>', $fail_filename or die "Can't write $fail_filename: $!";

You are using three-argument open and lexical filehandles. That's great. Those tutorials are paying off.


die "$message\n";

If you include a trailing newline "\n" in your message to die, it will not tell you what line it died on. You are mixing both versions. I think you should stick with the line numbers. Get rid of those newline, and remember that it's important to be consistent in programming.


sub run_parallel {
    my $command_array_reference = shift;

I guess you read somewhere that variable names should be speaking. Or maybe you don't want to get confused about the kind of data this variable holds. Speaking names are great. But you don't really need to name your variable foo_array_reference. The fact that its sigil is a $ already tells you it's a scalar, and thus likely a reference. A better name would be $commands, where the plural indicates it's a list of some kind. If you want to write down that it's an array, write a comment or some POD over the function.

=head2 run_parallel

Takes an array reference of commands, spawns four workers to work through
all the commands in parallel and returns the return 
value of C<$manager->wait_all_children>.

=cut

sub run_parallel {
    my $commands = shift;

As you can see from my documentation, we have another issue coming up.


unless ((ref $command_array_reference) =~ m/ARRAY/) {
    print "run_parallel requires an array reference as input.\n";
    die;
}

There is a bit too much stuff here. You don't need a regular expression match to check the ref. It will return "ARRAY", so you could say:

ref $commands eq 'ARRAY'

However, that will give a use of unitialized value warning if we pass a string or a number, or something else that is not a reference. If you really want to check this, and don't want to write

unless ( ref $commands and ref $commands eq 'ARRAY') { ... }

you should take a look at Ref::Util, which provides a lot of useful functions to check this kind of thing in a very fast way, as it's written in XS. In your case, you could use is_arrayref.

If you insist on dieing, then do it properly. Don't print to STDOUT and then die without a message, so that it just says Died at line ... to STDERR. die can take an error message, but shouldn't have the newline \n, or it will not report the line number.

You can then use a nice post-fix unless.

use strict;
# ...
use Ref::Util 'is_arrayref';

sub run_parallel {
    # ...

    die 'run_parallel requires an array reference as input' 
        unless is_array($commands);

use Parallel::ForkManager;
my $manager = new Parallel::ForkManager(4);

When Perl executes your program, there are two phases. It first reads the whole thing and compiles it. This phase is called compile time. Then it actually runs the program, which is called run time. use statements are resolved during compile time. That means it doesn't matter if they are inside a sub, or an if. They will always be found and executed. Conditional use statements are a code smell. Put the use at the top with the others.

The old-fashioned new Module(@args) form (called indirect object notation) has been out of date for a long time. Instead, you should use the more modern version, which will take care of inheritance properly.

my $manager = Parallel::ForkManager(4);

my $START_DIRECTORY = getcwd();

ALL_CAPS variable names usually indicate constants in Perl. Your variable is lexical. Name it $start_directory instead.

You don't have to do the parenthesis after a function call if Perl knows it's a function (it was imported from Cwd, so it knows that). It's ok to do them, but it's important to be consistent if you do, and have them all the time. You do that, so this is fine.


foreach my $command (0..scalar @{ $command_array_reference }-1) {
    # ...
    my ($dir, $cmd) = @{ $command_array_reference->[$command] };
    # ...
}

You are making your life harder than you have to here. Using a range and foreach is bettern than a C-style for loop, but it's not the easiest choice in this situation.

I would iterate the array reference directly. Instead of calling it in scalar context, just walk over its elements. The dereferencing operator @{} returns a list of all of them if you put it directly into the parens. This will make it easier to grab the two elements and might have saved you the question over on Stack Overflow.

foreach my $command (@{ $commands }) {
    # ...
    my ($dir, $cmd) = @{ $command };
    # ...
}

By the way, if you have a Perl that's at least version 5.24 you can use the post-fix dereferencing operator. It's easier to read because you can go left to right all the way, but if you're used to the old stuff (like me) it's hard to get used to it. Also, my commercial Perl IDE doesn't highlight it yet.

foreach my $command ( $commands->@* ) {
    my ($dir, $cmd) = $command->@*;
}

my @commands;

$commands[0][0] = 'tmp1';#directory
$commands[0][1] = 'echo "ave Maria" > aveMaria.txt';#command

$commands[1][0] = 'tmp2';#directory
$commands[1][1] = 'echo "IN HOC SIGNO VINCES" > xp.txt';#command

You can write these kinds of structures the same way Data::Dumper outputs them. That makes it easier to read.

my @commands = (
    [
        'tmp1', # directory
        'echo "ave Maria" > aveMaria.txt', # command
    ],
    [
        'tmp2', # directory
        'echo "IN HOC SIGNO VINCES" > xp.txt', # command
    ],
);

Also note that there is a convention to have space in front and after the # for comments, as that makes it easier to read.

In general, I prefer to use references directly when I know I'll take a reference of the variable. Especially for configuration. And since you never actually do anything else with @commands, there is no need to create it in the first place.

run_parallel(
    [
        [
            'tmp1',                               # directory
            'echo "ave Maria" > aveMaria.txt',    #command
        ],
        [
            'tmp2',                                   # directory
            'echo "IN HOC SIGNO VINCES" > xp.txt',    #command
        ],
    ]
);

Maintainability

You have some code duplication in the signal handler and execute. Opening and writing the file is implemented twice, with only the same error message being different. It would be nicer to write a function that does all of that, and pass it the message. You can then use that function in the signal handler as well as execute.

There is also no need to explicitly close, as $fh is lexical and will be cleaned up automatically when it goes out of scope, or in this case, the program ends.

sub log_error_and_die {
    my $error = shift;

    my $fail_filename = "$TOP_DIRECTORY/$0.FAIL";
    open my $fh, '>', $fail_filename or die "Can't write $fail_filename: $!";
    print $fh $error;

    die $error;
}

And you can call it like this

# kill the program if there are any warnings
local $SIG{__WARN__} = sub {
    my $message = shift;
    log_error_and_die( sprintf( '%s @ %s', $message, getcwd() ) );
};

and

sub execute {
    my $command = shift;
    print "Executing Command: $command\n";
    if (system($command) != 0) {
         log_error_and_die("$command failed.");
    }
}

Why are there always four jobs? If there are a lot of files, wouldn't it be easier to make this an option? Especially with your limited input data, four jobs are overkill. But they might be not enough.

I would add a second argument to run_parallel and pass the number of parallel jobs.

sub run_parallel {
    my ($commands, $jobs) = @_; # alternate way to get arguments
    # ...

    my $manager = Parallel::ForkManager($jobs);
}

Of course, since there is nothing else, you can also get rid of that function all together. If you only ever call it once from the body of your script, I believe there's no need to make it into a function. Opinions vary on this, though.

Other considerations

    chdir $dir or die "Can't chdir to $dir: $!";
    execute( $cmd );
    chdir $START_DIRECTORY or die "Can't chdir to $START_DIRECTORY: $!";

Why do you chdir before calling execute? Is that not part of the execution? I think I would move that into execute.

Furthermore, why do you chdir back afterwards? That's already the working directory of the child, which will be reaped a few lines down. At that point, its working directory doesn't matter, and it will never have any influence on the parent's or the sibblings' working directory. You can safely remove that second chdir.

Once you've done that, you can pass $command along to execute and have a cleaner interface.


$commands[0][0] = 'tmp1';#directory
$commands[0][1] = 'echo "ave Maria" > aveMaria.txt';#command

The inner data structure of your commands could be a hash reference as well. That would make it a bit more work to write it all down, but you would not need those comments any more, and it will be way easier to read half a year later.

my $commands = [
    {
        dir => 'tmp1',
        cmd => 'echo "ave Maria" > aveMaria.txt',
    },
    # ...
];

Then to work with it in your loop, you don't have to assign $dir and $cmd any more.

sub execute {
    my $command = shift;
    chdir $command->{dir} 
        or die "Can't chdir to $command->{dir}: $!"; 

    print "Executing Command: $command->{cmd}\n";
    if (system($command->{cmd}) != 0) {
         log_error_and_die("$command->{cmd} failed.");
    }
}

# ...

foreach my $command (@{ $commands }) {
    $manager->start and next;
    execute($command);
    $manager->finish;
}

Instead of all the die calls you can use the autodie pragma, which will turn all failures into die. It can make the code a lot more readable.

use autodie 'all'; # includes system()

Instead of the signal handler, you can set all warnings to be FATAL on the warnigns pragma.

use warnings FATAL => 'all'; # every warning dies
\$\endgroup\$
  • 2
    \$\begingroup\$ Where are the upvotes? This answer is outstanding. \$\endgroup\$ – t3chb0t Dec 5 '17 at 0:03
  • 1
    \$\begingroup\$ @t3chb0t: I think simbabque knows that I tend to be sparing with my encouragement. I am very binary: something is either right or it is wrong, and I will generally upvote only if something is exceptional or insightful, while I will downvote anything that is just wrong. \$\endgroup\$ – Borodin Dec 5 '17 at 0:57
  • 1
    \$\begingroup\$ @Borodin I belive you... 13 votes in nearly 4 years speaks volumes ;-P \$\endgroup\$ – t3chb0t Dec 5 '17 at 1:10
  • 1
    \$\begingroup\$ @simbabque: You're very good at this, but because you've written so very much I'm struggling to find a way to critique what you've said and add to what you've missed out. No one reading another answer, or a comment on your answer, will be inclined to scroll through your words to compare and contrast other people's ideas. This has become a code review review, but I really think that you've stifled other people's ideas by bringing a banquet to a buffet. \$\endgroup\$ – Borodin Dec 5 '17 at 1:14
  • 1
    \$\begingroup\$ @Borodin you're right that this is long, and not everyone will even read it. I considered splitting it up into two answers, but then that's hard to read for the OP. On code review I think it's more personal than on SO. Other people come here to learn rather than to look for solutions, and they come by their own accord. While on SO it's all about building a knowledge repository, here it's focusing on the person asking to have their code reviewed. And I've done that. I think about the OP first, and I hope they will read it. I encouraged them to post, so I'm confident they will read this. \$\endgroup\$ – simbabque Dec 5 '17 at 6:56

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.