Hi Steve, here are some comments on the code on the list (seeing that I commented on this thread and now have to comment). I hope it's OK - if it's not - we can take it in private.
On Tue, 10 Apr 2012 08:25:04 -0400 Steve Scaffidi <[email protected]> wrote: > Indeed, I've made some changes in that direction already. The code lives in > github (user Hercynium) as IPC::Pipeline::Composable. I think I now know what > the classes will look like, and I will create those while preserving the > existing exportable functions 'cause they've grown on me. > > Classes will also make some things easier to test, too! > > The code might be a good candidate for review/hacking/critique at a Boston.pm > meeting, and I welcome any suggestions via this list or my own email, of > course. > In https://github.com/Hercynium/IPC-Pipeline-Composable/blob/master/lib/IPC/Pipeline/Composable.pm : sub ipc_pipeline { my ($class, @procs) = !eval { $_[0]->isa(__PACKAGE__) } ? (__PACKAGE__, @_) : blessed($_[0]) ? (__PACKAGE__, @_) : @_; Seems like $procs[0] will always be $_[0] except for the last case. Furthermore, the two first results are identical and should be merged. Maybe this code is too clever for its own good. Later on in the same function you have: return $class->new( procs => [ map { # IPC::Process object can be used as-is eval { $_->isa("${class}::Process") } ? $_ : # IPC object needs its processes extracted eval { $_->isa($class) } ? ($_->procs) : # If it's a non-ref, try to stringify and use that as the process spec ! ref($_) ? ${ \"${class}::Process" }->new(cmd_str => $_) : # Initialize a Process object from an arrayref reftype($_) eq 'ARRAY' ? ${ \"${class}::Process" }->new(cmd => shift(@$_), args => $_) : # Initialize a Process object from a subref reftype($_) eq 'CODE' ? ${ \"${class}::Process" }->new(cmd => $_) : # unknown argument? DIE! die "unhandled type passed to ipc_pipeline!\n"; # TODO: handle placeholders for processes } @procs ]); } I think all this is too complex to be placed in map when it's not extracted. Furthermore, I also think that you risk cloberring "$_", and I would suggest assigning it to a lexical variable. Finally, why are you using ${ \"..." } instead of just "..."? Moving on: [CODE] # allow exported subs to be called as functions, or object/class methods. sub __func_meth_args { return !eval { $_[0]->isa(__PACKAGE__) } ? (__PACKAGE__, @_) : # function call blessed($_[0]) ? (__PACKAGE__, @_) : # object method call @_; # class method call } [/CODE] This seems like duplicate code to what I've commented about before, and I believe that a CGI.pm-like subroutines that are callable either as methods or as non-OOP functions and procedures is not a good idea. If you do want to provide a procedural wrapper, then you should put it in another namespace. [CODE] # This is so non-portable... I can't find a CPAN dist for this??? sub __proc_path_from_fh { return $OSNAME =~ /Linux/i ? "/proc/self/fd/".fileno(shift) : $OSNAME =~ /MacOS|darwin/i ? "/dev/fd/".fileno(shift) : return; } [/CODE] You have a small amount of duplicate code there and you're also doing "return return;", which doesn't make a lot of sense. Since this code also only supports Linux and Mac OS X, I would suggest taking a look at the source for GNU Bash and/or similar portable shells and see how they are doing it. You may also wish to consider extracting this functionality into its own self-contained CPAN distribution. That's it for now, but I haven't reviewed the code thoroughly. As a general impression, I would say that it seems a little too clever for my taste. Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ "The Human Hacking Field Guide" - http://shlom.in/hhfg Confucius says: “XSLT made me realise humanity was hopeless.”. Please reply to list if it's a mailing list post - http://shlom.in/reply . _______________________________________________ Boston-pm mailing list [email protected] http://mail.pm.org/mailman/listinfo/boston-pm
