patternMinor

Bowling calculator in Perl

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

bowlingcalculatorperl

Problem

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 <= $#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

Solution

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 <= $#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

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

Code Snippets

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

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

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

# 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];
}

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

Context

StackExchange Code Review Q#41893, answer score: 8

Revisions (0)

No revisions yet.