Is this the way to go about building Perl subroutines?

Question

So I've had a simple ucwords function for Perl which I've had a while, and wanted to expand it, this is what I've come up with, is this the way I should be building my functions to handle optional parameters?

Original:

sub ucwords{
    $str = @_[0];
    $str = lc($str);
    $str =~ s/\b(\w)/\u$1/g;
    return $str;
}

Extended:

sub ucwords{
    if(@_[0] ne undef){#make sure some argument was passed
        @overloads = (0,1,2,3);
        $str = @_[0];
        if(@_[1] eq undef || @_[1] eq 0){ #default is to lowercase all but first
            $str = lc($str);
            $str =~ s/\b(\w)/\u$1/g;
            return $str;
        }else{ #second parameters
            if(!grep $_ eq @_[1], @overloads){ die("No overload method of ucwords() takes ".@_[1]." as second parameter."); }
            if(@_[1] eq 1){ $str =~ s/\b(\w)/\u$1/g;} #first letter to upper, remaining case maintained
            if(@_[1] eq 2){ $str = lc($str); $str =~ s/(\w)\b/\u$1/g;} #last letter to upper, remaining to lower
            if(@_[1] eq 3){ $str =~ s/(\w)\b/\u$1/g;} #last letter to upper, remaining case maintained
            return $str;
        }
    }else{
        die("No overload method of ucwords() takes no arguments");
    }
}

Psy

Answer 1

In one word: NO!

Let's look at:

sub ucwords{
    $str = @_[0];
    $str = lc($str);
    $str =~ s/\b(\w)/\u$1/g;
    return $str;
}

First and foremost, you are not using strict. Use it. It is for your own good.

Second, you are not using warnings. Use it. It is for your own good. For example, the first element of @_ should be referred to using $_[0] and not @_[0].

Third, you should get in the habit of reading the FAQ list occasionally before re-inventing the wheel again: See How do I capitalize all the words on one line?

If you think this is harsh, consider the fact that when called as:

print ucwords("FRED AND BARNEY'S LODGE"), "\n";

your code outputs

Fred And Barney'S Lodge

which is the example given in that question.

Further, having a function that does more than one thing, chooses what it does on the basis of mystery numbers and does none of those things right is not a good design strategy.

You should instead have multiple functions, named in a way that can be understood by a casual reader of your code, each of which does only one thing and does that right.

Finally, the extended version of your function (without saying anything about the wisdom of writing such a function) can be better written as:

# untested code follows

use Carp;

{
    my %modes = map {$_ => undef} 0 .. 3;
    sub ucwords{
        croak 'No arguments passed' unless @_;

        my ($str, $mode) = @_;
        $mode = 0 unless defined $mode;

        croak "Invalid mode: '$mode'" unless exists $modes{$mode};

        if ( $mode == 0 ) {
            $str = lc($str);
            $str =~ s/\b(\w)/\u$1/g;
        }
        elsif ( $mode == 1 ) {
            $str =~ s/\b(\w)/\u$1/g;        
        }
        elsif ( $mode == 2 ) {
            $str = lc($str); 
            $str =~ s/(\w)\b/\u$1/g;        
        }
        else {
            $str =~ s/(\w)\b/\u$1/g;
        }

        return $str;
    }
}

See also Why use if-else if in C++?

Answer 2

Don't use the $foo ne undef construct. Operators in Perl are what's known as "context sensitive". By using certain operators, you introduce certain contexts. ne, eq, lt, gt, le, ge are all "string" operators, treating the scalars on either side as strings, whereas ==, !=, <, >, <=, >= are numeric operators, treating the object on either side as a number.

However, if you're testing for undef, it really doesn't make sense that something undefined is a number or a string, so they have an operator just for that kind of test: defined

You can test if something is defined simply by doing

if (defined $foo) {
    # my cool logic on $foo here
}