"Guess my Number" game

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);
        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}.";
    }

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 =  );

    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 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:

• Use whitespace. Whitespace is good and improves readability.

• use warnings. use 5.016 should already be activating strict for you. More warnings = more bugs found early.

• 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().

• 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.

• 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.

-
You currently have this sequence of conditions inside the loop:

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

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

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.

-
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.

• The $n is always zero. You don't increment it. This is good, as it should be the same as

StackExchange Code Review Q#23550, answer score: 11