Re: Looking for pointers or optimizations.
by 2teez (Vicar) on Aug 21, 2012 at 13:47 UTC
|
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
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
#!/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.
Take my advice. I don't use it anyway.
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
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. | [reply] [Watch: Dir/Any] [d/l] [select] |
Re: Looking for pointers or optimizations.
by greengaroo (Hermit) on Aug 21, 2012 at 13:39 UTC
|
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!
Take my advice. I don't use it anyway.
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
| [reply] [Watch: Dir/Any] [d/l] |
|
open (my $inputFH, "<", $filename)
or die "cannot open input file: $!";
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
|
| [reply] [Watch: Dir/Any] |
|
| [reply] [Watch: Dir/Any] |
Re: Looking for pointers or optimizations.
by aaron_baugher (Curate) on Aug 21, 2012 at 13:52 UTC
|
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.
Aaron B.
Available for small or large Perl jobs; see my home node.
| [reply] [Watch: Dir/Any] [d/l] |
Re: Looking for pointers or optimizations.
by choroba (Cardinal) on Aug 21, 2012 at 13:48 UTC
|
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.
| [reply] [Watch: Dir/Any] |
|
open (my $fh, "<", $ARGV[0]) or die "Can't open input file!";
my @words;
while (<>) {
push(@words, $_);
}
| [reply] [Watch: Dir/Any] [d/l] |
|
Better, but not yet correct: you should tell Perl which file handle to read from:
while (<$fh>) {
| [reply] [Watch: Dir/Any] [d/l] |
|
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, $_);
}
| [reply] [Watch: Dir/Any] [d/l] |
|
|
|
Re: Looking for pointers or optimizations.
by tobyink (Canon) on Aug 21, 2012 at 13:55 UTC
|
#!/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");
perl -E'sub Monkey::do{say$_,for@_,do{($monkey=[caller(0)]->[3])=~s{::}{ }and$monkey}}"Monkey say"->Monkey::do'
| [reply] [Watch: Dir/Any] [d/l] |
Re: Looking for pointers or optimizations.
by hbm (Hermit) on Aug 21, 2012 at 14:23 UTC
|
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]/ ...
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
my $displayed;
foreach my $s (split(//, $word)) {
$displayed .= exists($correct_guesses{$s}) ? "$s " : "* ";
}
| [reply] [Watch: Dir/Any] [d/l] |
Re: Looking for pointers or optimizations.
by hbm (Hermit) on Aug 21, 2012 at 20:26 UTC
|
Looking at your 3:11 PM version, I'd make a few more changes:
- You split(//,$word) twice, perhaps every time through the while loop. Move them up, prior to looping.
- 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 | [reply] [Watch: Dir/Any] [d/l] [select] |
Re: Looking for pointers or optimizations.
by aitap (Curate) on Aug 21, 2012 at 14:10 UTC
|
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
Sorry if my advice was wrong.
| [reply] [Watch: Dir/Any] [d/l] [select] |