14
\$\begingroup\$

I have a Perl application that allows the user to choose between two different formats for data output (and there will likely be more in the near future). In the heart of the algorithm, the code makes a call to a print subroutine.

my $stats = analyze_model_vectors( $reference_vector, $prediction_vector );
print_result( $stats, $tolerance );

The print_result subroutine simply calls more specific methods.

sub print_result
{
  if($outformat eq "text")
  {
    print_result_text(@_);
  }
  elsif($outformat eq "xml")
  {
    print_result_xml(@_);
  }
  else
  {
    # Should not reach this far if input checking is done correctly
    printf(STDERR "Error: unsupported output format '%s'\n", $outformat);
  }
}

Is this good practice? What other alternatives are there and what are their pros/cons? I could think of the following alternatives.

  • Test for output format in the heart of the algorithm, and call the appropriate printing subroutine there.
  • I've never used subroutine references before, but perhaps when I could store a reference to the correct subroutine in a scalar variable and call the print method with that scalar in the heart of the algorithm.
  • Place code for all output formats in a single subroutine, separated by if/elsif/else statements.

Keep in mind there may be more output formats required in the near future.

\$\endgroup\$
  • 1
    \$\begingroup\$ Big if/elsif/else chains are ugly. The canonical method of handling this case in Perl is to use a dispatch table: a hash containing subroutine references. See Charles Bailey's response. A tempire offers a more exotic approach, that uses Perl's internal symbol tables for the lookup. \$\endgroup\$ – daotoad May 30 '11 at 18:26
  • \$\begingroup\$ How/where is $outformat specified? \$\endgroup\$ – 200_success Oct 13 '14 at 18:33
15
\$\begingroup\$

If you can pass in the subroutine it makes the code a lot simpler, you also don't have to deal with an unknown string format as the subroutine itself has been passed in.

sub print_result
{
    my $subroutine = shift;
    &$subroutine( @_ );
}

print_result( \&print_result_text, $arg1, $arg2 )

Otherwise I think I'd go with with a hash of subroutine references. It's easily readable and simple to update.

sub print_result
{
    my %print_hash = ( text => \&print_result_text,
                        xml => \&print_result_xml );

    if( exists( $print_hash{ $outformat } ) )
    {
        &{$print_hash{ $outformat }}( @_ );
    }
    else
    {
        printf(STDERR "Error: unsupported output format '%s'\n", $outformat);
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I think I'll try using the subroutine references. \$\endgroup\$ – Daniel Standage Jan 22 '11 at 17:15
6
\$\begingroup\$

I find that Perl's flexibility can help you eliminate many IF/ELSIF/* code constructs, making code much easier to read.

sub print_result {
    my ($stats, $tolerance, $outformat) = @_;

    my $name = "print_result_$outformat";

    print "$outformat is not a valid format" and return
      if !main->can($name);

    no strict 'refs';
    &$name(@_);
}

sub print_result_xml { ... }
sub print_result_text { ... }
sub print_result_whatever { ... }


Walkthrough

print "$outformat is not a valid format" and return if !main->can($name);

This checks the main namespace (I presume you're not using classes, given your code sample) for the $name subroutine. If it doesn't exist, print an error message and get out. The sooner you exit from a subroutine, the easier your code will be to maintain.

 no strict 'refs';

no strict 'refs' turns off warnings & errors that would be generated for creating a subroutine reference on the fly (You're using 'use strict', aren't you? If not, for your own sanity, and for the children, start). In this case, since you've already checked for it's existence with main->can, you're safe.

 &$name(@_);

Now you don't need any central registry of valid formatting subroutines - just add a subroutine with the appropriate name, and your program will work as expected.

If you want to be super hip (some might say awesome), you can replace the last 5 lines of the subroutine with:

no strict 'refs';
main->can($name) and &$name(@_) or print "$outformat is not a valid format";

Whether you find that more readable or not is a simple personal preference; just keep in mind the sort of folk that will be maintaining your code in the future, and make sure to code in accordance with what makes the most sense to them.

Perl is the ultimate in flexibility, making it inherently the hippest language in existence. Make sure to follow http://blogs.perl.org and ironman.enlightenedperl.org to keep up with the latest in Modern::Perl.

On a separate note, it's Perl, not PERL. The distinction is important in determining reliable & up-to-date sources of Perl ninja-foo.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why not use eval? eval { &$name(@_); 1 } or print "$outformat is not a valid format\n"; can() may be fooled by autoloaded functions and other magics, but simply trying the call is as authoritative as it gets. \$\endgroup\$ – daotoad May 30 '11 at 18:30
  • \$\begingroup\$ eval is brute force. Calling the function outright assumes it's idempotent and has no unintended side effects. Also, it seems to me errors should always be avoided. Using eval/try/catch is for the !@#$ moments that should never happen. \$\endgroup\$ – tempire Jun 3 '11 at 3:39
5
\$\begingroup\$

I'd use anonymous subroutines to make the code cleaner:

my %output_formats = (
    'text' => sub {
        # print_result_text code goes here
    },
    'xml' => sub {
        # print_result_xml code goes here
    },
    # And so on
);

sub print_result {
    my ($type, $argument1, $argument2) = @_;

    if(exists $output_formats{$type}) {
        return $output_formats{$type}->($argument1, $argument2);
    } else {
        die "Type '$type' is not a valid output format.";
    }
}
| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

I try to avoid Perl if I can, so this is more of a general answer: I've coded like this in an old VB6 app. Each output function had a wrapper function that then called the required implementation using a series of IFs. Sometimes a particular output method wouldn't need anything - eg. "Start New Line" is relevant for text file output, but not Excel worksheet output.

I'm currently in the process of porting/re-writing that particular app in C#/.NET 4, where I've been able to take a much more object oriented approach. I have defined an "output base class" with a standard interface. This is then inherited by the various implementations. As I start the output, I can create the required implementation using a factory method/class, and pass data to it using the standard interface.

This particular implementation is actually multi-threaded, so the bulk of the output base class is actually the thread & queue management. Data is then passed in using a small "data chunk" class, and queued for output using a thread-safe queue.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The OO approach is definitely making things easier to handle, conceptually and practically. Thanks! \$\endgroup\$ – Daniel Standage Jan 22 '11 at 17:16
  • 2
    \$\begingroup\$ Can't downvote this yet, but the OOP approach is completely overkill and unnecessary here. Languages like VB6 and Java (not too sure about C#) don't have first-class functions so that you are supposed to do some OOP magic. Perl has first-class functions and thus you can use them as any other variable. The answer provided by Charles Bailey is thus more correct and also more appropriate for the provided code. \$\endgroup\$ – Nikolai Prokoschenko Feb 9 '11 at 13:06
  • 1
    \$\begingroup\$ Seriously, I would down-vote this twice if I could. ( I don't have enough rep to down-vote yet ) \$\endgroup\$ – Brad Gilbert Dec 16 '11 at 23:57

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.