in reply to Perl replacement for choice.com

You ran into a pet peeve of mine (at least twice in presented code):

There shall be no else after leaving enclosing scope (exit, die, return, last …)

When using if … else …, the code after that is to be executed always, that is why you have if/else. If however the first part ENDs with an exit of the enclosing scope, there is no need whatsoever for the else. Removing else, the braces and the indent, make the code easier to read and maintain.

if (expression) {
    …
    exit;
    }
else {
    …
    …
    }

=>

if (expression) {
    …
    exit;
    }

…
…

[download]

See for slides from a talk here and here/here:

sub foo
{
    my $s = shift;

    if (defined $s) {

        if ($s ne "") {
            #
            # 150 lines of code
            #
            }
        else {
            die "String is empty";
            }
        }
    else {
        die "String is null";
        }
    }

=>

sub foo
{
    my $s = shift;

    unless (defined $s) {
        die "String is null";
        }

    if ($s eq "") {
        die "String is empty";
        }

    #
    # 150 lines of code
    #
    }

[download]

Enjoy, Have FUN! H.Merijn

Replies are listed 'Best First'.
Re^2: Perl replacement for choice.com
by onelander (Sexton) on Mar 04, 2013 at 18:19 UTC

    I believe I know one area you were talking about but I am not certain of the second. Removing all the comments, is this what you mean? 

    if ( defined $default ) {
    my $stderr;
    close STDERR;
    open(STDERR, ">", \$stderr);

    my $grabkey_thread = threads->create(\&grabkey);
    $grabkey_thread->detach();

    for ( my $i = 0; $i <= $timeout; $i++ ) {
        sleep 1;
    }
    
    my $pos = index( uc( $choices ), uc( $default ) ) + 1;
    print uc( $default ) . "\n";
    exit $pos;
    
} else {
    grabkey();
}

=>

if ( defined $default ) {

    my $stderr;
    close STDERR;
    open(STDERR, ">", \$stderr);
    
    my $grabkey_thread = threads->create(\&grabkey);
    $grabkey_thread->detach();

    for ( my $i = 0; $i <= $timeout; $i++ ) {
        sleep 1;
    }
    
    my $pos = index( uc( $choices ), uc( $default ) ) + 1;
    print uc( $default ) . "\n";
    exit $pos;
    
}

grabkey();

    [download]
[reply]
[d/l]

      yes, you caught both

      It is a bit of a shame you edited the original posted code, as now "new" readers will not see the changes you made based on the comments you got, and the comments now make no sense at all (for those people).

      As you learn from perlmonks, readers that stumble on this post no wdo not learn from the code changes. (FWIW By learning from changes I do not imply that all changes are always correct, but discussion about them makes you take desicions that are more thought-through.


      Enjoy, Have FUN! H.Merijn
[reply]
        What is the best way to handle updates to the code? I assume leave the original code as is and then in the comments show the updates. Where is the best place to put the fully corrected version of the code?
[reply]
Re^2: Perl replacement for choice.com
by onelander (Sexton) on Mar 04, 2013 at 18:20 UTC
    Thanks for the tip too. Being able to write better code is one reason I enjoy this site.
[reply]
Re^2: Perl replacement for choice.com
by onelander (Sexton) on Mar 04, 2013 at 22:14 UTC

    I found the two areas you were talking about and updated the code to reflect those changes.

[reply]

In Section Cool Uses for Perl