Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question

3 Answers 3

up vote 7 down vote accepted

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 <= 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
share|improve this answer
    
Impressive answer! However, I do not quite agree with the way you removed the ternary operator in the first functions as I find OP's version easier to read. Pure matter of personal preference I guess. –  Josay Feb 17 at 17:18
    
The ternary operator is just a personal style choice. I seek out using it whenever possible because it's my favorite operator :) This answer was extremely helpful and revealed to me some syntax that I was not aware existed. –  SirBraneDamuj Feb 17 at 18:36
    
Since function parameters are aliased within @_ (and not copied anywhere before making the call?) is it safe to say that there is no real advantage to passing an array ref to function (as opposed to passing an array as list)? –  mpapec Feb 17 at 19:44
1  
@mpapec There are many subtle differences. Calling a function with a list of scalars means that this list of scalars has to be written onto the stack, whereas an arrayref is just a single item. This becomes noticeable for really large amounts of data. Using an arrayref also allows us to modify the array itself (e.g. to append an element), whereas using a list of aliased scalars only allows us to modify each element. When writing list transformations (as here), a flat list is preferable as it's easier to compose. In other cases, references are preferable because of performance. It depends. –  amon Feb 17 at 20:04

Tests

Your test cases only involve zeros and tens. Surely you need to test more realistic, less exotic cases too.

Input format

I like that you disregard the - in the input and interpret the framing yourself — that enhances robustness. Common notation for a spare is a / on the second throw. It would be nice if you supported that. As @rolfl points out, you need to support that because each throw has to be represented in one character, and you have no other way to indicate a frame of 0, 10. Also, in accordance with the principle that you should be lenient in what you accept, it would be better to accept both uppercase and lowercase x as a strike.

Parameter passing

You split the tasks into small functions, each with a specific purpose. Normally, that is an excellent idea, but I feel that the parameter passing is very cumbersome when expressed in Perl.

First, I recommend consistently unpacking the parameter list into named variables in the first line of each subroutine. That way, it's easy to see what parameters the subroutine expects. An exception could be made if the subroutine is trivial (e.g. is_strike() below). In contrast, your score_frame() has a $_[0] buried in the last line, which hurts readability.

Second, Perl is a punctuation-heavy language, and you managed to get hit hard, both by dereferencing and by array access, whenever you use @{$_[0]}[$foo..$bar] — which is quite frequently. My previous suggestion to give names to all parameters helps a bit. An object-oriented interface for frames would help — you could write $game->throws($foo..$bar) instead.

Another strategy is to pass the lists by value rather than by reference.

You currently call score_frame(\@throws, $current).

How about calling score_frame(@throws, $current) instead? Unfortunately, that would make it awkward for score_frame() to unpack its arguments:

sub score_frame {
    my (@throws) = @_;
    my $current = pop @throws;
    …
}

It would be better to put the scalar parameter first: score_frame($current, @throws). Then, the arguments could be unpacked more naturally:

sub score_frame {
    my ($current, @throws) = @_;
    …
}

But we can make it even simpler. Instead of maintaining and passing the index of the first throw of the frame, just consume the throws, shrinking the list a frame at a time. The front of the list is always the first throw of the current frame. See my solution below — I think it feels really natural!

Decomposition

The functions is_strike(), is_spare(), and score_frame() are a good idea. However, I feel that frame_advance() and throws_for_score() actually hurt readability, since it's no longer apparent from looking at the main loop in score_game() what the three cases are.

I prefer to interpret the input using this one-liner in score_game(). It gets the assignment right the first time, requiring no subsequent mutation.

# Split the game history into an array of throws,
# replacing X with 10.
… = map { s/X/10/i; $_} split(/-?/, $game);

Suggested solution

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

# Is this frame a strike?
sub is_strike { 
    shift == 10;
}

# Is this frame a spare? (Call this subroutine only if it's not a strike.)
sub is_spare {
    my ($first_throw, $second_throw) = @_;
    $second_throw eq '/' || $first_throw + $second_throw == 10;
}

sub score_frame {
    my ($first_throw, $second_throw, @bonus) = @_;
    $second_throw = 10 - $first_throw if $second_throw eq '/';
    sum($first_throw, $second_throw, @bonus);
}

sub score_game {
    my ($game) = @_;

    # Split the game history into an array of throws,
    # replacing X with 10.
    @_ = map { s/X/10/i; $_} split(/-?/, $game);

    my $score = 0;
    for my $frame (1..10) {
        if (is_strike(@_)) {
            # Consume one throw in this frame.
            # Include the next two throws as a bonus for this frame.
            $score += score_frame(shift, $_[0], $_[1]);
        } elsif (is_spare(@_)) {
            # Consume two throws in this frame.
            # Include the next throw as a bonus for this frame.
            $score += score_frame(shift, shift, $_[0]);
        } else {
            # Consume two throws in this frame.  No bonus.
            $score += score_frame(shift, shift);
        }
    }
    $score;
}

say score_game("X-X-X-X-X-X-X-X-X-X-XX"); #300
say score_game("55-55-55-55-55-55-55-55-55-55-5"); #150
say score_game("00-00-00-00-00-00-00-00-X-X-XX"); #60
say score_game("01-10-20-03-04-35-55-6/-X-24"); #77
share|improve this answer
2  
Nice review. Note that { s/foo/bar/; $_ } is equivalent to s/foo/bar/r which is available since perl 5.14 (released 2011). Reassigning to @_ is frowned upon (except when doing tail calls), because it doesn't buy you anything over creating a new variable: my @frames = map ... is more self-documenting. –  amon Feb 17 at 23:38

I have a number of problems with your program, and amon has categorized most of them.... but, the most significant issue is that the code does not work... at least when it comes to spares, and some un-tested input values....

There are three places it fails:

  • it does not specify a good way to declare a spare. I would have expected a token like / to indicate a spare, leading to input like:

    print score_game("5/-5/-5/-5/-5/-5/-5/-5/-5/-5/-5") . "\n"; #150
    

    But, in the absence of that token, it makes it impossible to input the 'wild' spare sequence that I would have represented as:

    print score_game("0/-0/-0/-0/-0/-0/-0/-0/-0/-0/-0") . "\n"; #100
    
  • this leads to the second issue I have, which is invalid input..... you do not handle it very well. For example, should the spare sequence have been input as:

    print score_game("010-010-010-010-010-010-010-010-010-010-0") . "\n"; #100
    

    your code strips the frame-separating - hyphen before it is sure what the frame really is.....

  • finally, your code accepts broken data as input:

    print score_game("66-66-66-66-66-66-66-66-X-X-XX") . "\n"; #156
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.