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.

I have recently just started learning Perl, so I decided to program the classic 'Guess my Number' game. I was wondering if anyone can spot any areas for improvement. As it's my first program, I expect there to be some unoptimized areas.

#!\usr\bin\perl  
print"Welcome to guess my number!\n";
    $compNumber=int rand 100;
    while($n<6){
        print"Guess a number between 1 and 100: ";
        chomp($userNumber=<STDIN>);
        if($compNumber>$userNumber){
            print"Your guess was too low, try again.\n";
        }
        if($compNumber<$userNumber){
            print"Your guess was too  high, try again.\n";
        }
        $n++
    }
    if($compNumber==$userNumber){
        print"Well done, $compNumber was my number! You got it correct in $n guesses!";
    }
    if($compNumber!=$userNumber){
        print"Unfortunately, you couldn't guess my number in 6 guesses, it was actually ${compNumber}.";
    }
share|improve this question
add comment

2 Answers 2

up vote 11 down vote accepted

First, always use strict; use warnings; at the top of your code. The strict pragma restricts you to a sensible subset of Perl, which removes many sources for errors. Also, it forces you to declare all your variables with my. The warnings are very helpful, but non-fatal errors.

If you are on a fairly modern perl (10 or later), you can use the say function. This works exactly like print, but appends a newline at the end. Enable this feature with use feature 'say' or use 5.010.

If you have a set of mutually exclusive conditions in an if-cascade, you might want to look into the if/elsif/else structure.

Your while-loop is meant to execute up to six times. Instead of while, you might want to use a range, and a foreach loop:

for (1 .. 6) {
  # this stuff executes six times
}

Inside your loop, you only test if the user didn't hit the number. If his guessed number is equal to the secret num, then you continue with a new iteration. Don't.

Also, I would pack the whole algorithm into a subroutine. This is a good habit, and allows us to do an early exit:

sub guess_my_number {
  my %args = @_; # cast the subroutine arguments to a hash
  my $maxnum = $args{maxnum} || 100; # unpack named arguments,
  my $tries =  $args{tries}  ||   6; #   or give default value

  my $secret = int rand $maxnum;

  GUESS:  # a labeled loop
  for my $try (1 .. $tries) {
    say "Guess a number between 0 and $maxnum:";

    chomp( my $guess = <STDIN> );

    if ($guess == $secret) {
      say "Yay, that was right! $secret was my number.";
      say "You managed in $try tries.";
      return 1; # exit the subroutine
    } elsif ($guess < $secret) {
      say "Your guess was too low.";
    } elsif ($guess > $secret) {
      say "Your guess was too big."
    }
    # This is executed for both too low or too large numbers:
    say "You have ", $tries - $try, " tries left.";
  }

  # we land here if we didn't manage the guess
  say "You failed to guess the correct number $secret in $tries guesses.";
  return 0;
}

And then in the main code:

guess_my_number(maxnum => 50, tries => 5);

All arguments are optional.

The above might produce a session like

Guess a number between 0 and 50:
25
Your guess was too low.
You have 4 tries left.
Guess a number between 0 and 50:
37
Your guess was too low.
You have 3 tries left.
Guess a number between 0 and 50:
44
Your guess was too big.
You have 2 tries left.
Guess a number between 0 and 50:
40
Your guess was too low.
You have 1 tries left.
Guess a number between 0 and 50:
42
Yay, that was right! 42 was my number.
You managed in 5 tries.

What you could (or should) implement is input validation. Without strict and warnings, Perl will transform any string into a number, which will be zero for non-numeric strings. To assert that the input only contains ASCII digits, run a regex over it:

if (not $guess =~ /^[0-9]+$/) {
  say qq(Your guess "$guess" was not a number.); # special qq() quotes
  redo GUESS; # redo this iteration, without incrementing the counter
}

About your updated code:

  1. Use whitespace. Whitespace is good and improves readability.
  2. use warnings. use 5.016 should already be activating strict for you. More warnings = more bugs found early.
  3. We don't call subs like &foo except when you know what you are doing. This style has a number of side effects that don't matter for your example, but can introduce bugs in complex applications. Always prefer foo().
  4. Why on earth are you using our? The our is just like my, except that it declares globals. Don't use globals. Use lexicals with my. In either case, declare your variable once and use it throughout the remaining scope. I.e. declare the variable outside the loop.
  5. If you look at all your execution paths, you test for duplicate conditions: $compNumber == $userNumber is tested two times. Look at my code again, which manages with one test. The real reason I used a sub is that I could return early, as soon as the correct number was guessed. This made my loop control easier.
  6. You currently have this sequence of conditions inside the loop:

    if    $compNumber == $userNumber
    elsif $compNumber >  $userNumber
        if $_ == 5
            last
        else
    elsif $compNumber <  $userNumber
        if $_ == 5
            last
        else
    

    As soon as $compNumber != $userNumber, we can test for the last iteration. This structure is logically equivalent, but shorter:

    if    $compNumber == $userNumber
    elsif $_ == 5
        last
    elsif $compNumber >  $userNumber
    elsif $compNumber <  $userNumber
    

    In my code, I simply drop of the last iteration of the loop, after telling the user if his guess was too small or too large, as usual.

  7. You use the topic variable $_. This makes for short code, but also makes it hard to understand. Consider using a better name, like $try or $guess. The topic variable is the topic of the current code (you can often pronounce it this or it). Your topic is the guess the user made, not the number of the iteration.

  8. The $n is always zero. You don't increment it. This is good, as it should be the same as your loop variable. As I print the congratulatory message inside the loop, I can use the loop variable, which will yield the correct value.
  9. My subroutine argument unpacking code may seem a bit arcane if you are new to Perl, but using hardcoded constants is an anti-pattern in any language. (Also, named arguments make for really nice interfaces.)
share|improve this answer
1  
@Lewis I added a section discussing your updated code. Some aspects are partially personal preferences (using whitespace, no &foo invocation, no globals, no duplication), but these have good reasons. –  amon Mar 8 '13 at 8:11
add comment

You could ask the user for a range between the random number should be choosen. Also, you check is the user is right after he has tried 6 times, but what if he has right in less than 6 tries ? You better should write something like

boolean won = false
int tries = 0

while (tries < MAX_TRIES || won == false)
{
    /* ask for a number and compare it, if it is the right number, set win to true */
}
share|improve this answer
add comment

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.