Bowling calculator in Perl A bowling score calculator is my go-to exercise for learning the basics of a language. I recently had to pick up Perl so I did this.

I'm concerned that I'm doing subroutines wrong. I had a hard time figuring out the syntax for dealing with arguments and what exactly @_ is.

I'm just looking for feedback on any egregious mistakes or instances where I go against convention.

use strict;
use warnings;
use List::Util qw(sum);

sub is_strike { #is this frame a strike?
    $_[0] == 10;
}

sub is_spare { #is this frame a spare?
    ($_[0] + $_[1]) == 10;
}

sub throws_for_score { #how many extra throws contribute to this frame's score?
    (is_strike(@_) || is_spare(@_)) ? 2 : 1;
}

sub frame_advance { #how many throws should we consume to move to the next frame?
    is_strike(@_) ? 1 : 2;
}

sub score_frame { #sum the score for x throws, where x is the number of throws that count for this frame's score
    my $frame_index = $_[1];
    my $frame_end = $frame_index + 2;
    my $score_end = $frame_index + throws_for_score(@{$_[0]}[$frame_index..$frame_end]);
    sum @{$_[0]}[$frame_index..$score_end];
}

sub score_game {
    (my $game = $_[0]) =~ s/-//gi; #remove hyphens
    my @throws = split(//, $game); #create an array of throws
    find_strikes(\@throws);
    my $current = 0;
    my $score = 0;
    do {
        my $frame_score = score_frame(\@throws, $current);
        $score += $frame_score;
        $current += frame_advance(@throws[$current..($current+2)]);
    } while($current &lt;= $#throws-2); #while the current index is not beyond the tenth frame
    $score;
}

sub find_strikes { #replace strike marks with 10
    foreach(@{$_[0]}) {
        if($_ eq 'X') {
            $_ = 10;
        }
    }
}

print score_game("X-X-X-X-X-X-X-X-X-X-XX") . "\n"; #300
print score_game("55-55-55-55-55-55-55-55-55-55-5") . "\n"; #150
print score_game("00-00-00-00-00-00-00-00-X-X-XX"); #60
It's nice that you're using strict and warnings.

Subroutine Arguments

A subroutine should first unpack the argument list, e.g. like my ($foo, $bar) = @_;, then do some processing, and explicitly return a value. While we can return implicitly and can access items in @_ directly, both can be error-prone and should be avoided in most situations.

For example, this would change the is_strike to

sub is_strike {
    my ($frame) = @_;
    return $frame == 10;
}


The line (is_strike(@_) || is_spare(@_)) ? 2 : 1; is similar obfuscation. First, we'll get rid of the unnecessary ternary operator:

return 2 if is_strike(@_) || is_spare(@_);
return 1;


Next, we make it clearer by unpacking the arguments here as well. Don't mind the implied copy, we are optimizing for readability not performance!

my ($frame_1, $frame_2) = @_;
return 2 if is_strike($frame_1) || is_spare($frame_1, $frame_2);
return 1;


Why is this better? We immediately see that this function only uses its first two parameters.

We can do the same exercise with score_frame. You are using the @_ argument array throughout the whole subroutine, instead of giving each argument a self-explaining name.

# sum the score for x throws,
# where x is the number of throws that count for this frame's score
sub score_frame {
    my ($frame, $index) = @_;
    my $frame_end = $index + 2;
    my $score_end = $index + throws_for_score(@$frame[$index .. $frame_end]);
    return sum @$frame[$frame_index .. $score_end];
}


Let's look at this excerpt:

(my $game = $_[0]) =~ s/-//gi; #remove hyphens
my @throws = split(//, $game); #create an array of throws


The /i flag (case insensitive) on the substitution makes no sense, as you aren't using any characters that have a case. Try to avoid /i, as it makes many optimizations in the regex engine impossible.

If you are only deleting a set of characters, you can use the more efficient transliteration tr/-//d, where d is “delete”. Together with proper argument unpacking, we get:

my ($game) = @_;
$game =~ tr/-//d;
my @throws = split //, $game;


Whether you use parens for the arguments to builtin functions is largely a matter of taste. I prefer to omit them as I find the resulting code cleaner (and faster to write). Others enjoy that parens make the argument list explicit, which is otherwise hard to understand (technically, Perl parsing is not decidable).

This line is hard to understand:

while($current &lt;= $#throws-2); #while the current index is not beyond the tenth frame


I don't know what the tenth frame is about, but wouldn't a condition like $current &lt;= 10 be better if this is about the tenth frame?. 

The map Function

The find_strikes function has a number of problems. Here is it's current form:

sub find_strikes { #replace strike marks with 10
    foreach(@{$_[0]}) {
        if($_ eq 'X') {
            $_ = 10;
        }
    }
}



Modifying data that has been passed in is generally frowned upon. If you do something like that, document it. Even better, return a new list.
Whenever possible, name the iteration variable instead of using $_.
Some people prefer the statement if condition form instead of the verbose if (condition) { statement }. Note that the braces are not optional in this case.


For example, we could rewrite the code like this:

sub find_strikes {
    my ($frames) = @_;
    my @frames_with_strikes;

    for my $frame (@frames) {
        if($frame eq 'X') {
            push @frames_with_strikes, 10;
        }
        else {
            push @frames_with_strikes, $frame;
        }
    }

    return @frames_with_strikes;
}


That is excessively verbose, and actually a good example where the ?: operator can be set to good use:

sub find_strikes {
    my ($frames) = @_;
    my @frames_with_strikes;

    for my $frame (@frames) {
        push @frames_with_strikes, ($frame eq 'X') ? 10 : $frame;
    }

    return @frames_with_strikes;
}


We can shorten this further by using a map list transformation instead of an explicit loop:

sub find_strikes {
    my ($frames) = @_;
    return map { $_ eq 'X' ? 10 : $_ } @$frames;
}


It is more “perlish” to take a list of values rather than an array reference:

sub find_strikes {
    my (@frames) = @_;
    return map { $_ eq 'X' ? 10 : $_ } @frames;
}


or without unnecessary copies:

sub find_strikes {
    return map { $_ eq 'X' ? 10 : $_ } @_;
}


In either way, it could now be invoked as

my @frames = find_strikes(split //, $game);


Less action at a distance, and clearer code.

say > print

You have lines like print score_game("X-X-X-X-X-X-X-X-X-X-XX") . "\n". This can be improved


… by passing a list rather than a single string:

print score_game("X-X-X-X-X-X-X-X-X-X-XX"), "\n";


all arguments will be joined without a space (unless you change certain settings).
… by using say instead of print. The say function behaves exactly the same, but automatically appends a newline. Prefer it for normal text output. Use print when you don't want to print out a line, e.g. when handling binary data, a protocol with specific requirements on line endings, or when printing half a line.

use feature qw/say/;  # activate the "say" function, available since v5.10.0

say score_game("X-X-X-X-X-X-X-X-X-X-XX");



Testing

In comments to your print invocations, you have given the expected output. Let's make an automated test out of this!

The Test::More module is Perl's most commonly used test framework. Here is how we can write a test:

use Test::More;

is(score_game("X-X-X-X-X-X-X-X-X-X-XX"),          300, '"X"es');
is(score_game("55-55-55-55-55-55-55-55-55-55-5"), 150, 'fives');
is(score_game("00-00-00-00-00-00-00-00-X-X-XX"),   60, 'zeroes and "X"es');

done_testing;


The is test compares for string equivality (which is also good enough for numbers). There is also the ok test which asserts that some condition is true. The last argument is an optional test description.

When we now execute the script, we get the following output:

ok 1 - "X"es
ok 2 - fives
ok 3 - zeroes and "X"es
1..3


everything passed, nice! If I'm sneaky and change the expected output of one of the functions, we get:

ok 1 - "X"es
not ok 2 - fives
#   Failed test 'fives'
#   at so1.pl line 54.
#          got: '150'
#     expected: '42'
ok 3 - zeroes and "X"es
1..3
# Looks like you failed 1 test of 3.


The is test outputs a reason for the test failure when the input doesn't match our expectations.

The output format is called TAP (Test Anything Protocol), and rarely used manually. Test suites are usually executed with the prove tool, which ships with Perl:

$ prove some_test.pl
some_test.pl .. ok   
All tests successful.
Files=1, Tests=3,  0 wallclock secs ( 0.04 usr  0.00 sys +  0.02 cusr  0.00 csys =  0.06 CPU)
Result: PASS