How should I handle errors in Perl methods, and what should I return from the methods?

Question

Hi

I've wrapped Perl's Net::SSH::Expect with a small module to reduce the boilerplate code needed to write a new configuration script for use with our HP iLO cards. While on one hand I want this wrapper to be as lean as possible, so non-programmer colleagues can use it, I also want it to be as well-written as possible.

It's used like so:

my $ilo = iLO->new(host => $host, password => $password);
$ilo->login;

$ilo->command("cd /system1");
$ilo->command("set oemhp_server_name=$system_name", 'status=0');

and this is iLO::command():

sub command {
    my ($self, $cmd, $response) = @_;

    $response = 'hpiLO-> ' unless defined($response);

    # $self->{ssh} is a Net::SSH::Expect object
    croak "Not logged in!\n" unless ($self->{ssh});

    $self->{ssh}->send($cmd);
    if ($self->{ssh}->waitfor($response, $self->{CMD_TIMEOUT}, '-re')) {
        return {
            before => $self->{ssh}->before(),
            match => $self->{ssh}->match(),
            after => $self->{ssh}->after(),
        };
    } else {
        carp "ERROR: '$cmd' response did not match /$response/:\n\n",
            $self->{ssh}->before()),
            "\n";
        return undef;
    }
}

I have two related queries. First, how should I deal with responses that don't match the expected response? I guess what I'm doing now is satisfactory -- by returning undef I signal something broke and my croak() will output an error (though hardly gracefully). But it feels like a code smell. If Perl had exceptions I'd raise one and let the calling code decide whether or not to ignore it/quit/print a warning, but it doesn't (well, in 5.8). Perhaps I should return some other object (iLO::response, or something) that carries an error message and the contents of $ilo->before() (which is just Net::SSH::Expect's before())? But if I do that -- and have to wrap every $ilo->command in a test to catch it -- my scripts are going to be full of boilerplate again.

Secondly, what should I return for success? Again, my hash containing more-or-less the response from Net::SSH::Expect does the job but it doesn't feel 'right' somehow. Although this example's in Perl my code in other languages emits the same familiar smell: I'm never sure how or what to return from a method. What can you tell me?

Answer 1

The usual way to raise exceptions in Perl is with die. The usual way to catch them is using eval with a block as an argument, and testing $@ after the eval finishes.

Answer 2

If you're familiar with exceptions from languages like Java, then think of die as throw and eval as try and catch. Instead of returning undef, you could do something like this:

if ($self->{ssh}->waitfor($response, $self->{CMD_TIMEOUT}, '-re')) {
    return {
        before => $self->{ssh}->before(),
        match => $self->{ssh}->match(),
        after => $self->{ssh}->after(),
    };
}

die "ERROR: '$cmd' response did not match /$response/:\n\n" 
. $self->{ssh}->before();

Then, in your calling code:

eval { 
    $ilo->command("set oemhp_server_name=$system_name", 'status=0');
};

if ( my $error = $@ ) { 
    # handle $error here
}

Just like exceptions in other languages, this allows you to bail out of a submethod at any point without having to worry about propagating return values up the call stack. They'll be caught by the first eval block that finds them. Additionally, you can die again to rethrow an exception you can't deal with back up the stack.

Even better, you can use die to throw an object, which your exception handler can interrogate for useful information and error messages. I like to use Exception::Class for this purpose. The Error module provides some syntactic sugar for doing Java-like try/catch blocks, as well.

Answer 3

In addition to using "die" as exception, you can also add another method:

if (!$ilo->commandSucceeded("set oemhp_server_name=$system_name", 'status=0')) {
     #recover here
}

Of course, the internal implementation of command() becomes

die ... if !commandSucceeded;

Answer 4

You'll find a lot of discussion of this sort of thing in googlespace. The best practice, no matter what you decide, is to not overload any of the values so the return value means different things. It should always be an error code, or it should never be the error code. People shouldn't have to look at the actual value to decide if it is an error code or not.

Check out the popular Perl modules on CPAN (or the ones you already use) to see what they do. I even talk about this a little in Mastering Perl, but I don't give a black-and-white answer. As with all real code, the real answer is "It depends".

There are a lot of different ways to do this. Unfortunately, that means people do it in every way. Since that is the case, I invoke consistency as the paramount rule. What does most of the code already do (not counting the wrong way)? If I have to fit into an existing codebase, I try to use the same sort of interface that most of the code already uses.

If there is no clear winner, write use cases using a couple of different styles. Which one fits the problem better or more naturally expresses the steps most users will take? That's not always just reading for die and eval. Write sample scripts using your unimplemented interfaces. Which style are you going to want to use? I found that actually writing the script before I implement the interface shows me a lot more than I was thinking about. If I'm writing stuff for other people to use, I show them the scripts in different styles and ask them which one they like better.

And, if all of that fails, reach for the 2d6. :)

Answer 5

I find that using exceptions improves the quality of the code dramatically. Even if Joel does not agree with this :)

It's been shown that any code using conditional statements to check for every return tends to be more buggy, which I did find to be true in practice. Many times.... More code, more bugs. Bah.

You do have to think through your exception strategy first - it's more difficult, but it's much easier later - if you get it right.

I use Error::Simple for this. Has been working for me for years.