Thursday, 11 June 2009

lc(x) - possible bad practice?

use strict; use warnings;

We all use the above, right? Well, certainly we should, and my team does;

Now this always causes a warning to occur when testing an undefined value (exception - if the test is for it to be undef).

my $arrayref = $self->method_returning_array_ref() || [];

Now, assuming that the method will always return an arrayref or undef, I will have an arrayref.

Now, it is common enough (returning stuff from an XML dom for example) that there is actually only 1 thing in the array, so I test against it

if ($arrayref->[0] eq 'yes') {
do something..
}

Now, if $arrayref->[0] is undef, this always throws an uninitialised variable warning. This normally leads me to change to

if ($arrayref->[0] && $arrayref->[0] eq 'yes') {
do something..
}

so that I don't spam up the logs with warnings.

However, we discovered today that by lowercasing the $arrayref->[0] variable, this will turn an undef into an empty string (for the conditional), therefore dispensing with any warnings.

if (lc$arrayref->[0] eq 'yes') {
do something..
}

Is this good or bad coding practice?

Reasons for it to be good
- You do not need an extra conditional just to dispense with the warning
- code less

Reasons for it to be bad
- It doesn't seem right
- The conditional is no longer testing against the pure result
- Are we getting rid of the warning for the wrong reason?
- Does it read correctly?

At this time, it is not something that the code police (perlcritic) seem to think is a bad practice, and certainly will make less code for us. It just seems like we are breaking an unwritten coding rule.

No comments: