15:50 < rindolf> klync: I should note that I hate returning flattened hashes from functions. Returning a single hash-ref is much better, IMO.

mostly pretty good for a first-time perl module. some stylistic comments appear below. (unfortunately my tabs -- which are really hard spaces of course -- are getting clobbered for some reason).


package Parallel::Supervisor;
# good
use strict;
use warnings;
#
# don't bother with this for now:
#
# use 5;
#
# it's not needed, and not really meaningful unlesss you're
# aiming for backward compatability with some (recent-ish) version
# of Perl 5. In which case the syntax would be:
#
# require 5.6;
#
# or variants thereof. but again this is neither here nor there for now.

# ..

# just put the VERSION declaration on one line - don't bother with
# the extra BEGIN block unless there's a reason for it
our $VERSION = "0.01";


# rewritten to be more idiomatic
sub new {
my $class = shift;
return bless {
'STRUCTS' => {}, # children ready to run (idx name)
'PROCESSES' => {}, # children running (idx pid)
'FINISHED' => {}, # children finished running (idx name)
'NAMES' => {} # index on running child names (idx pid)
}, $class
}

## like the other person said, definitely return hash references,
## rather than full blown hashes

# returns hash of all the jobs prepared but not attached
sub structs {
my $self = shift;
return $self->{STRUCTS}
}

sub prepare {
# yes, there are mulitple ways to peal these off the stack; but
# often it's better to be clear by declaring a few interim args:
my ($self, $name, @cmd_args) = @_;
# btw it's better to do this join below, closer to where
# it's needed. you probably do want to verify that these
# args are allo defined, but other than that, thee basic
# syntax you chose was fine:
#
# my $cmd = join ' ', @cmd_args;
#
# however it could be a bit tighter:
my $cmd = "@cmd_args"; # same effect

return undef unless defined $name;
return undef if $self->is_ready($name);

# umm, you'll probably want to warn or confess (see the Carp module)
# on these child errors, yes?
my $child_writer = Symbol::geniosym;
return undef if $?;
# also, consider using English module:
use English;
my $parent_reader = Symbol::geniosym;
return undef if $CHILD_ERROR; # same thing

#### Setup IPC
# causes Illegal seek, but safe to ignore?
pipe( $parent_reader , $child_writer );

$parent_reader->autoflush(1);
$child_writer->autoflush(1);

# you might want to make your hashes a bit more tightly spaced.
# also, the single arrow, while not strictly required, does make
# things a bit more consistent wrt hash derefs:
$self->{STRUCTS}->{$name} = {
'id' => $name,
'cmd' => $cmd,
'child_writer' => $child_writer,
'parent_reader' => $parent_reader
};

return 1;
}

sub is_ready {
my ($self,$name) = @_;
# this is wrong! it will still boolean-eval to true
# return undef unless (keys(%{$self->{STRUCTS}} ) );
# the grep you used works of course, but is somewhat
# unsatisfactory in that it forces a new array creation.
# instead maybe try this:
for ( keys(%{$self->{STRUCTS}} ) {
return 1 if $_ eq $name
}
return undef;
}

#
# ... skipping on down ...
#

# returns a list with pid of all active processes
sub get_pids {
my $self = shift;
# also wrong, because the unless clause always boolean-evals to true
# return @pids unless ($self->{PROCESSES});
#
# even if the hash is empty. anyway since the effect
# of the boolean false case is to return an empty
# array, you can just do this:
return keys %{$self->{PROCESSES}}
}

# ... all I have time for ...

Wow, thanks for all these tips - time to give the module another pass!

I was hesitant to return a ref to ensure the records are only modified by the methods provided and not by the calling code, which could cause unpredictable behaviour depending on how complicated the calling code is - for example, where their names come from and whether they are recycled. Correct me if that's wrong, but it just seems safer to me, at the expense of having to copy a few bytes here and there.

Thanks again for a really helpful review!