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?

Radiator

Actually no return value is needed - see perldoc.perl.org/functions/return.html – Bligh

It's not needed but the reason perlcritic flags it, is that to use an implicit return is bad style. E.g. you end your script with $result = $a + $b; - and then someone wants to debug it, figuring out what $result is - they add a print after it. Which changes the return code of your script. Far better to be explicit about it. – Eeg

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.

Eeg

That makes perfect sense! Unfortunately in this case I'm working on a 'big script', so looks like I may need to re-write :) Thank you for the detailed answer and examples mate. – Radiator

It's awkward to retcon, and looks hard work initially - and it is - but it pays dividends as complexity increases, because you always know exactly where to look when there's a problem. – Eeg

Re "Regarding return values - perl implicitly returns the last result. That's a gotcha waiting to happen", Nonsense. – Khanate

Well, that's the reason perlcritic gives for flagging it. search.cpan.org/~thaljef/Perl-Critic-1.123/lib/Perl/Critic/… – Eeg

Answer 2

If the function doesn't mean to return anything, there's no need to use return!

No, you don't use any aspects of OO (encapsulation, polymorphism, etc). What you are doing is called procedural programming. Nothing wrong with that. All my work for nuclear power plants was written in that style.

The problem is using @main::array, and I'm not talking about the fact that you could abbreviate that to @::array. Fully-qualified names escape strict checks, so they are far, far more error-prone. Mistyped var name won't get caught as easily, and it's easy to have two pieces of code collide by using the same variable name.

If you're just using one file, you can use my @array, but I presume you are using @main::array because you are accessing it from multiple files/modules. I suggest placing our @array in a module, and exporting it.

package MyData;
use Exporter qw( import );
our @EXPORT = qw( @array );

our @array;

1;

Having some kind of hint in the variable name (such as a prefix or suffix) indicating this is a variable used across many modules would be nice.

---

By the way, if you wanted do create an object, it would look like

package MyArray;

sub new {
   my $class = shift;
   my $self = bless({}, $class);
   $self->{array} = [ @_ ];
   return $self;
}

sub get_elements {
   my ($self) = @_;
   return @{ $self->{array} };
}

sub sort {
   my ($self) = @_;
   @{ $self->{array} } = sort @{ $self->{array} };
}

sub push {
   my $self = shift;
   push @{ $self->{array} }, @_;
}

sub pop {
   my ($self, $n) = @_;
   return splice(@{ $self->{array} }, 0, $n//1);
}

my $array = MyArray->new(1,4,2,6,1,8,5,5,2);
$array->sort;
print("$_\n") for $array->get_elements();
$array->push_array(1..9);
$array->pop_array(3);

I improved your interface a bit. (Sorting shouldn't print. Would be nice to push different things and to pop other than three elements.)

Khanate

I'm entirely happy with doing something procedural rather than OO. I think I'd still tend to avoid globals as much as possible though, just to minimise namespace pollution and action at a distance. – Eeg

@Sobrique, It's not really have procedural without the globals. Maybe you mean functional? Honestly, people forget how global objects are. An object can be modified by any module to which the object is passed, giving it virtually the same scope as a global variable. The OP shouldn't be making every variable global, only the major data structures. Think of it as inside-out objects. – Khanate