David Pashley.com

Perl Style and readability

Which of these two fragments is more readable?

$self->{catalina_base} = $ENV{'CATALINA_BASE'};
if (!defined $self->{catalina_base}) {
    $self->{catalina_base} = $self->getTomcatHome() ;
}
if (!defined $self->{catalina_base}) {
    CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3);
}

or

$self->{catalina_base} = $ENV{'CATALINA_BASE'} || $self->getTomcatHome()
   || CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3);

Update: or

$self->{catalina_base} = (
   $ENV{'CATALINA_BASE'}
   or $self->getTomcatHome()
   or CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3)
);

Filed under computing

Tagged perlstyle

7 comments

Comments are closed. These are preserved from the original site.

Cyril Brulebois —
Hi. You should have a look at the (IMHO) excellent book "Perl Best Practices". It'd have suggested something like :
$self... = (
  Choice1
  or Choice2
  or Choice3
);

Definitely, the "Update" choice was the best of the initial proposals.

Cheers.
Decklin Foster —
You should express the semantics, not the method of calculation.

$self->{catalina_base} = $ENV{'CATALINA_BASE'} || $self->getTomcatHome();
CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3) unless
$self->{catalina_base};

Second line should wrap, of course. And really that error message needs work. What are CATALINA_BASE and TOMCAT_HOME? What is the difference between "unset" and "undefined" and why am I supposed to care? Does that mean one is an environment variable and one is not? How am I supposed to tell? What were you trying to do at the time? (I don't want to grep the documentation for all occurences of those names.)

Testing definedness doesn't make sense; you assigned something to it right up at the top. Just test what that value is. undef is false.
Jon —

Why would the third be better than the second?

Robert Thau —
Mu.  They aren't functionally equivalent.  The first will honor the value of $ENV{CATALINA_BASE} even if it is set to the empty string; the second will not.  And if the environment variable really is undefined, and getTomcatHome() returns the empty string, the first will honor that, while the second will cough up the error message.  I'm not sure which behavior is more desirable, but they are different.  ('' is a defined value, but it's one of several that Perl treats as a logical false.  This is more commonly a pitfall with numeric zeroes).

As far as readability goes, I'd say that the "or" forms are easier on the eyes.
Jno —

Whilst I can appreciate 'or' is more readably then '||', the use of indentation on the third example leads me to think that some kind of tuple is being assigned to $self->{catalina_base}.

Alex —
Using or / || in place of defined() is usually bad style; you confuse definedness with truth - whether or not you can afford to do that depends on the situation, and is therefore a bad idiom.

You can get a // operator which does the ||-thing, but on definedness rather than truth.

I would proffer:

$self->base = (defined $ENV{'catbase'})? $ENV{'catbase'} : $self->getBase();
CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3) unless(defined $self->base);

... or something, on the basis it does the definedness test correctly, but the implicit fall-back isn't great.
joost —
Clearly the third is better, as it gives me the warm perl fuzzies.

BTW, the error handling is not part of the assignment, so IMHO this is even better:

$self->{catalina_base} = (
  $ENV{'CATALINA_BASE'}
  or $self->getTomcatHome()
) or CCM::Util::error ("CATALINA_BASE unset and TOMCAT_HOME undefined", 3);