Looking for pointers or optimizations.

[thmsdrew]

Hi gang!

I just wrote a Hangman script in Perl. It pretty much portrays the extent of my Perl knowledge. This script is just about as good as I am when it comes to Perl, if that makes sense. I'm wondering if you guys can review my code, give me any criticism, inform me of any optimizations I'm missing out on, etc. I don't really care for "Perl golf"-style code, but I do care about optimized, neat code. Readability is important.

Thanks in advance!

Updated as of 3:11 PM EDT.

#!/usr/bin/perl -w
use strict;

open(my $fh, "<", $ARGV[0]) or die "cannot open input file: $!";
my @words = <$fh>;
close $fh or die "cannot close input file: $!";
my $words = @words;
chomp(my $word = lc($words[int(rand($words))]));

my (%correct, $guesses);
my $turns = 8;
while ($turns > 0) {
    my $displayed;
    foreach my $char (split(//, $word)) {
        $displayed .= exists($correct{$char}) ? "$char " : "* ";
    }
    if (index($displayed, "*") < 0) {
        print "\nYou win! You guessed $word!\n";
        exit;
    }
    
    print "\n$displayed\n",
          "Fails remaining: $turns\n",
          "Your guesses: $guesses\n",
          "Next guess: ";
    
    chomp(my $guess = <STDIN>);
    $guess = lc($guess);
    
    if ($guess !~ /[a-z]/ || length($guess) != 1 || index($guesses, $g
+uess) >= 0) {
        print "\nInvalid guess.\n";
    }
    else {
        my %word = map { $_ => 1 } split(//, $word);
        if (exists($word{$guess})) {
            $correct{$guess}++;
        }
        else {
            $turns--;
        }
        $guesses .= "$guess ";
    }
}
print "\nYou lost! The word was $word!\n";

[2teez]

Hi,
• You might want to use "use warnings" instead of "-w", so that you can remove warning using "no warnings" pargma, where you wouldn't want some.
• Always check the return for the open function like so: open $fh,'<',$filename or die "can't open file: $!" OR you use use autodie qw(open close);
• The File handler $fh decleared was not used
• filehandle $fh was not close. You should do that like so:close $fh or die "can't close file:$!"
You really might what to try out the module "perlcritic", based on Damian Conway's book Perl Best Practices. Check Perlcritic
Using it from Command Line Interface like so:
perlcritic -3 perlscript.pl

[greengaroo]

Please correct me if I am wrong (sometimes I am) but I think that no warnings; works for the -w switch too. I was asking myself the same question a while ago. Try this:

#!/usr/bin/perl5.10 -w

my %hash1 = {
  test1 => 'test1',
  test2 => 'test2'  
};

no warnings;
my %hash2 = {
  test1 => 'test1',
  test2 => 'test2'  
};

use warnings;
my %hash3 = {
  test1 => 'test1',
  test2 => 'test2'  
};

I get:

Reference found where even-sized list expected at ./test_warnings.pl l
+ine 3.
Reference found where even-sized list expected at ./test_warnings.pl l
+ine 15.

Now the question is, what is the difference if any between the -w switch and use warnings;?

Update: Answering myself, on perldoc it says: The warnings pragma is a replacement for the command line flag -w , but the pragma is limited to the enclosing block, while the flag is global.

[hbm]

I very recently encountered a spurious(?) error message (on v5.8.2):

Use of uninitialized value in substitution (s///) at (eval 2) line 21

Which resulted from the addition of one line like this:

   $hash{$var1}{'string'} = $var2;

I confirmed that the variables were defined, and that the hash entry was created. I then tried:

   { no warnings 'uninitialized'; 
     # and later:  no warnings;
     $hash{$var1}{'string'} = $var2;
   }

To no avail. Curiously, the error wasn't occurring at that line (confirmed with print statements); but it disappeared if I commented out that line.

Removing -w got rid of the error.

[aaron_baugher]

You have some strange things going on at the top of your file. Comments interspersed:

my $words_file = @ARGV;

# Evaluating an array in scalar context returns the length
# of the array.  So if you are passing your script one filename,
# $words_file will equal '1', not the first argument from the 
# command line.  Pass it two filenames, and it will equal 2, etc.  
# But that doesn't matter, because of another bug:

open (my $fh, ">", $words_file);

# Here you open the file for writing, but never use it.
# (You should also check for errors, or use autodie.)

my @words;
while (<>) {
    push(@words, $_);
}

# Which turns out also not to matter, because you then use 
# the <> operator, which reads from the files in @ARGV 
# automagically.  So lines 4 & 5 (the first two I quoted), 
# though broken, don't affect anything else, and could be 
# discarded.

[greengaroo]

First, you should always explicitly close any filehandle, even if we all know it closes itself at the end of the script, it is good practice to close it as soon as you don't need it. About your elsif and else, you should start them on a new line.

Either:

}
elsif ( ... ) {

}
else {

}

or C style:

}
elsif ( ... )
{

}
else
{

}

But don't mix both in your code! Choose one and always use the same.

Take some space, its free! If readability is important to you, don't be afraid to add empty lines, like before and after a loop statement or a if-elsif-else block.

Also, if you want to learn more, I recommend you read the book Perl Best Practices (O'Reilly). Not only they provide guidelines but also they explain why. Enjoy!

[nemesdani]

"First, you should always explicitly close any filehandle"
...not to mention checking your filenhandle in the first place ;)

open (INP, "<", "$filename") or die "cannot open input file";
Another thing: you use @ARGV in scalar context - I doubt that this is what you want.

[MidLifeXis]

open (INP, "<", "$filename") or die "cannot open input file";

You may want to

• not interpolate the variable, just use it as is - why restringify something that you think should already be a string.
• use lexical file handles - it will allow it to fall out of scope rather than being a global. Forcing yourself to think of the scoping of variables can be good when trying to make your code modular.
• include the error message in the error response - it will help to debug why the file was not able to be opened. Was it due to too many files being opened, permissions being wrong on the file, the file not existing, sunspots...?

Update: I see that the OP has done two of the three of these already -- this comment is targeted at the parent post. :-)

open (my $inputFH, "<", $filename)
    or die "cannot open input file: $!";

[thmsdrew]

Thanks for the tips. I didn't really intentionally write my statements like that. Just kind of happened. Will check that book out!

[greengaroo]

Check out the thread I have started, if you haven't seen it yet... 20 most important Perl Best Practices. There are some very interesting thoughts.

[choroba]

Does the script work? I doubt it. You open the file for writing, not reading. Also, you are getting the number of arguments instead of the file name into $words_file. Oh, that explains why it works. Look for an empty file named 1.

[thmsdrew]

It worked fine but yes there was an empty file named 1.

If I change it to the following it should work as I meant it to:

open (my $fh, "<", $ARGV[0]) or die "Can't open input file!";
my @words;
while (<>) {
    push(@words, $_);
}

[choroba]

Better, but not yet correct: you should tell Perl which file handle to read from:

while (<$fh>) {

[2teez]

open (my $fh, "<", $ARGV[0]) or die "Can't open input file!";
my @words;
  ### The above die function should also return OS_ERROR if open funct
+ion failed
  ###  ... die "can't open file: $!";
while (<>) {
  ### <> will read from the STDIN not the filehandler $fh
  ### so that should be while(<$fh>){...}
    push(@words, $_);
}

[tobyink]

#!/usr/bin/env perl

use 5.010;
use strict;
use constant LIMIT => 8;

my $word      = uc "hello";
my $wrong     = 0;
my $already   = '';
my $displayed;

while ($wrong < LIMIT) {
    $displayed = join '',
        map { m{ ^ [ $already ] $ }x ? $_ : '*' }
        split '', $word;
    
    if ($displayed eq $word) {
        say "Yeah, baby, yeah!";
        exit(0);
    }
    
    say "Word:           $displayed",
    say "Wrong guesses:  $wrong";
    print "Please guess a letter...  ";
    chomp(my $guess = <>);
    $already .= ($guess = quotemeta uc $guess);
    $wrong++ unless $word =~ m{ $guess }x;
}

die("Dude, it was '$word'\n");

[hbm]

1. Instead of this:

    my $displayed;
    foreach my $s (split(//, $word)) {
        if (exists($correct_guesses{$s})) {
            $displayed .= $s;
        } else {
            $displayed .= "*";
        }
        $displayed .= " ";
    }

This?

        my $displayed = join" ",
                        map { exists $correct_guesses{$_} ? $_ : '*' }
                        split//, $word;

2. And instead of this:

    print "Fails remaining: $turns\n";
    print "Wrong guesses: ", join(" ", keys %wrong_guesses), "\n";
    print "Your guess: ";

This?

        print "Fails remaining: $turns\n",
              "Wrong guesses: ", 
              join(" ", keys %wrong_guesses), "\n",
              "Your guess: ";

3. And instead of this:

   if (!$guess =~ /[a-z]/ ...

This?

   if ($guess !~ /[a-z]/ ...

[thmsdrew]

In response to you first one, I have a similar optimization but a bit less confusing to me:

my $displayed;
foreach my $s (split(//, $word)) {
    $displayed .= exists($correct_guesses{$s}) ? "$s " : "* ";
}

[hbm]

Looking at your 3:11 PM version, I'd make a few more changes:

1. You split(//,$word) twice, perhaps every time through the while loop. Move them up, prior to looping.
2. You have @words, $words, $word, and %word. Below, I ditch the temporary $words, and introduce @chars and %chars.

...
#my $words = @words;  # just use (scalar @words)
chomp(my $word = lc($words[int(rand(scalar @words))]));
my @chars = split//,$word;
my %chars = map { $_ => 1 } @chars;
...
while ($turns > 0) {
   ...
   foreach my $char (@chars) { # split not needed
   ...
   else {
      #my %word = map{$_=>1}split(//,$word); # not needed
      if (exists($chars{$guess})) {  # changed %word to %chars
   ...

Minor update - tweaked my 'foreach' comment

[aitap]

In addition to the previous comments,

my $words_file = @ARGV;

You assign an array to a scalar. Array is treated in scalar context, and you have only number of arguments in it. Use this instead: my ($words_file) = @ARGV;. In this case an anonymous array gets assigned, and nobody gets harmed.

open (my $fh, ">", $words_file);
my @words;
while (<>) {
    push(@words, $_);
}

You read STDIN, not the $fh. Use <$fh>, not just <>. Also, in array context the <> operator returns the array with file contents. You can just run my @words = <$fh>;. See readline for more.

my $word = lc($words[rand($words) + 1]);

scalar @words returns count of the words; rand returns any number between (including) 0 and specified number (not including). You should also round this value using int: my $word = lc $words[int rand($words+1)];
_{Edit: my @words = <$fh>; # not $fh}