# Guess my Number in Perl

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 unomptimized (if that's a word) areas. So, here's the code:

#!\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}."; }  # Update After the answers left and working a few things out myself, here is my latest code: #!\usr\bin\perl use 5.016; &guess_the_number; sub guess_the_number{ say"Welcome to guess the number!"; my($n,$compNumber)=(0,int rand 100); GUESS: for(1..7){ print"Guess a number between 1 and 100: "; chomp(our$userNumber=<STDIN>);
if(not$userNumber=~/^[0-9]+$/){
redo GUESS;
}
if($compNumber==$userNumber){
last;
}
if($compNumber>$userNumber){
if($_==6){ last; } else{ say"Your guess was too low, try again."; } } if($compNumber<$userNumber){ if($_==6){
last;
}
else{
say"Your guess was too  high, try again.";
}
}
}
if($compNumber==our$userNumber){
say"Well done, $compNumber was my number! You got it correct in$n guesses!";
}
if($compNumber!=our$userNumber){
say"Unfortunately, you couldn't guess my number in 6 guesses, it was actually ${compNumber}."; } }  Any more improvements? Thanks. - add comment ## 2 Answers 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
}


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.)
-
@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
boolean won = false