Should a subroutine always return explicitly?

Question

If perlcritic says "having no returns in a sub is wrong", what is the alternative if they really aren't needed?

I've developed two apparently bad habits:

• I explicitly assign variables to the '$main::' namespace.
• I then play with those variables in subs.

For example, I might do..

#!/usr/bin/perl
use strict;
use warnings;

@main::array = (1,4,2,6,1,8,5,5,2);

&sort_array;
&push_array;
&pop_array;

sub sort_array{
    @main::array = sort @main::array;
    for (@main::array){
        print "$_\n";
    }
}

sub push_array{
    for ( 1 .. 9 ){
        push @main::array, $_;
    }
}

sub pop_array {
    for ( 1 .. 3 ){
        pop @main::array;
    }
}

I don't do this all the time. But in the above, it makes sense, because I can segregate the operations, not have to worry about passing values back and forth and it generally looks tidy to me.

But as I said, perl critic says its wrong - because there's no return..

So, is anyone able to interpret what I'm trying to do and suggest a better way of approaching this style of coding in perl? eg. am I sort of doing OOP?

Answer 1

In short - yes, you're basically doing OO, but in a way that's going to confuse everyone.

The danger of doing subs like that is that you're acting at a distance. It's a bad coding style to have to look somewhere else entirely for what might be breaking your code.

This is generally why 'globals' are to be avoided wherever possible.

For a short script, it doesn't matter too much.

Regarding return values - Perl returns the result of the last expression by default. (See: return)

(In the absence of an explicit return, a subroutine, eval, or do FILE automatically returns the value of the last expression evaluated.)

The reason Perl critic flags it is:

Require all subroutines to terminate explicitly with one of the following: return, carp, croak, die, exec, exit, goto, or throw.

Subroutines without explicit return statements at their ends can be confusing. It can be challenging to deduce what the return value will be.

Furthermore, if the programmer did not mean for there to be a significant return value, and omits a return statement, some of the subroutine's inner data can leak to the outside.

Perlcritic isn't always right though - if there's good reason for doing what you're doing, then turn it off. Just as long as you've thought about it and are aware of the risks an consequences.

Personally I think it's better style to explicitly return something, even if it is just return;.

Anyway, redrafting your code in a (crude) OO fashion:

#!/usr/bin/perl
use strict;
use warnings;

package MyArray;

my $default_array = [ 1,4,2,6,1,8,5,5,2 ];

sub new {
   my ( $class ) = @_;
   my $self = {};
   $self -> {myarray} = $default_array;
   bless ( $self, $class );
   return $self;
}

sub get_array { 
   my ( $self ) = @_;
   return ( $self -> {myarray} ); 
}

sub sort_array{
    my ( $self ) = @_;
    @{ $self -> {myarray} } = sort ( @{ $self -> {myarray} } );
    
    for ( @{ $self -> {myarray} } ) {
        print $_,"\n";
    }
    return 1;
}

sub push_array{
    my ( $self ) = @_;
    for ( 1 .. 9 ){
        push @{$self -> {myarray}}, $_;
    }
    return 1;
}

sub pop_array {
    my ( $self ) = @_;
    for ( 1 .. 3 ){
        pop @{$self -> {myarray}};
    }
    return 1;
}

1;

And then call it with:

#!/usr/bin/perl

use strict;
use warnings;

use MyArray;

my $array = MyArray -> new();

print "Started:\n";
print join (",", @{ $array -> get_array()} ),"\n";

print "Reshuffling:\n";
$array -> sort_array();

$array -> push_array();
$array -> pop_array();

print "Finished:\n";
print join (",", @{ $array -> get_array()} ),"\n";

It can probably be tidied up a bit, but hopefully this illustrates - within your object, you've got an internal 'array' which you then 'do stuff with' by making your calls.

Result is much the same (I think I've replicated the logic, but don't trust that entirely!) but you have a self contained thing going on.