Superlman has asked for the wisdom of the Perl Monks concerning the following question:

Having puttered about with Perl just long enough to have a general idea of what's going on, I've achieved a reasonable level of competentcy.

But, of course, that's not good enough

Below is a simple prime number program I wrote not long ago. What I'd like is for you illustrious monks to go over it and criticize (mock?) EVERYTHING. No matter how small or trivial. Is there a more efficient way to do $THING? Can my code be made easier to read / terse / faster / better smelling? Add features? Remove bloat?

You get the idea. Nitpick! Educate! Correct! (And a free ++ from me for each helpful comment, no matter how vicious) 

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

#These variables are user-configurable.
my $countermax = 10000;          #The max number of primes per file. (
+To regulate file size)
my $primefile = "Primes.seed";    #The location of the seed file. This
+ is needed.
my $path = "./";     #The first part of the path to dump the files in.
+ This must be pre-existing.
my $last = ".txt";                #The extension to append to the file
+names. Your choice.
my $beginloc = "00000";           #The number to start counting at whe
+n making filenames.

#End users should not edit below this line
my $lowrange = 0;
my $hirange = 0;
my $current = 0;
my $i = 0;
my $primey = 0;
my $counter = 0;
my @parray;
my $outputloc = $beginloc;

#First this looks for a pre-existing primes file
#which has all the primes less than 10000 in it. 
#It needs this file. Assume it is present or
#suggest a better way or something...
open PRIMES, $primefile;
while(<PRIMES>) {
push (@parray, $_);
}
close PRIMES;

open RESULTS, ">".$path.$outputloc.$last;

print "Welcome to the prime number program.\nThis program will tell yo
+u every prime number within a certain range.\nPlease enter the lower 
+range now: ";

$lowrange = <STDIN>;
chomp $lowrange;

#Simple idiot-proofing. I didn't bother to check to make
#sure it's actually an integer. What's a good way to 
#do this?
if($lowrange < 0) { $lowrange = 0; print "\nYour number must be positi
+ve. I have set it to 0 for you."; }

print "\nHow high should the program search for primes? ";
$hirange = <STDIN>;
chomp $hirange;

if($hirange <= $lowrange) { $hirange = $lowrange + 100;
print "\nYour second number must be at least equal to the first. \nI h
+ave arbitratily set it to be 100 greater.\n";
}


print "Now computing the range of all primes between $lowrange and $hi
+range. \nPlease wait.\n";

#No need to check even numbers, right?
if($lowrange % 2 == 0) { $lowrange++; }

#Of course, since 1 isn't a prime, we have to account
#for that. Mathmaticians, LART me if I'm wrong. 
if($lowrange == 1) { print RESULTS "2\n"; $lowrange = 3; $counter++; }

for($current = $lowrange; $current <= $hirange; $current += 2) {
$primey = 1;

for($i = 0; $parray[$i] <= sqrt($current); $i++) {
if($current % $parray[$i] == 0) { $primey = 0;
$i = 10000;
}
}


if($primey == 1) {
$counter++;
#print "PRIME FOUND: $current\n";
print RESULTS "$current\n";

if($counter%$countermax == 0) {
close RESULTS;
$outputloc++;
print "\nFile full. Currently ".($current *100/$hirange)."\% done.";

open RESULTS, ">".$path.$outputloc.$last;
}
}
}

print "\nTotal amount of primes: $counter \n";
print "Your results have been saved, starting with
".$path.$beginloc.$last."\n\n";

[download]

Replies are listed 'Best First'.
Re: Mock my code!
by Masem (Monsignor) on Sep 06, 2001 at 17:06 UTC
    First a "critical" error: you use an OS function "open" without checking for any error state. You should always, at minimal, use " or die $!" to print out some useful error message on these types of calls.

    I think the first thing to concern yourself with is style, assuming that the code above is not the result of errors introduced during cut and pasting. There's no indenting, which is very helpful in trying to identify loops. Other points where I see that it could be improved is adding more carriage returns to help separate a conditional/flow control statement from the actions that are taken; for example, you have: 

    if($lowrange < 0) { $lowrange = 0; print "\nYour number must be positi
+ve. I have set it to 0 for you."; }

    [download]
    It's must easier to read if you do: 
    if ($lowrange < 0) { 
    $lowrange = 0; 
    print "\nYour number must be positive. I have set it to 0 for you.
+"; 
}

    [download]
    Even a bit better is the following: 
    { 
    $lowrange = 0; 
    print "\nYour number must be positive. I have set it to 0 for you.
+"; 
} if ($lowrange < 0);

    [download]
    as this implies a non-critical conditional instead of a significant conditional.

    I'd also try to be consistent when you add your \n's for printing; either do them all at the end or all at the beginning; the mix is somewhat distracting.

    From a logic standpoint, you may want to round to the nearest int; if I enter "2.5" as the lower bound, the program won't find any primes, for example.

    In the search-for-primes loop, you can use the last keyword instead of setting $i to an incrediable high value as to leave the loop early.

    -----------------------------------------------------
    Dr. Michael K. Neylon - mneylon-pm@masemware.com || "You've left the lens cap of your mind on again, Pinky" - The Brain
    It's not what you know, but knowing how to find it if you don't know that's important

[reply]
[d/l]
[select]

      Good advice, Masem. But your if statement example doesn't work ... 

      do { # not without the do
    $lowrange = 0; 
    print "\nYour number must be positive. I have set it to 0 for you.
+"; 
} if ($lowrange < 0);

      [download]
      this form of if (or unless, foreach, while, until) only works on simple statements, not on blocks.

      -- Hofmator

[reply]
[d/l]
Re: Mock my code!
by Sifmole (Chaplain) on Sep 06, 2001 at 17:43 UTC
    A general comment -- some indentation of your code blocks would make things easier to read.
    ---------------------- 
    open PRIMES, $primefile;
while(<PRIMES>) {
push (@parray, $_);
}
close PRIMES;

# could be 
open PRIMES, $primefile;
@parray = <PRIMES>;
close PRIMES;

    [download]
    if($lowrange % 2 == 0) { $lowrange++; }

# could be 

$lowrange++ unless ($lowrange % 2);

    [download]
    Simple conditionals can be written as Masem pointed out. As well if you the unless you can take advantage of the fact that the result from $lowrange % 2 will be enough to tell TRUE or FALSE (1 or 0) without the additional '==' test.

    Also another note on prints: When you have something like 

    print "\nTotal amount of primes: $counter \n";
print "Your results have been saved, starting with
".$path.$beginloc.$last."\n\n";

    [download]
    You can use a single print 
    print "\nTotal amount of primes: $counter \n",
      "Your results have been saved, starting with
".$path.$beginloc.$last."\n\n";

    [download]
    Also you can use the variables inside the double quotes: 
    print "Your results have been saved, starting with
".$path.$beginloc.$last."\n\n";

# becomes
print "Your results have been saved, starting with ${path}${beginloc}$
+{last}\n\n";

    [download]
    The curly braces are not always required but they can alleviate some possible headaches.

    Just a few notes for ya.

[reply]
[d/l]
[select]
Re: Mock my code!
by dragonchild (Archbishop) on Sep 06, 2001 at 18:14 UTC
    I made a number of changes.
    1. Got rid of the primes file. Unless you're planning on working with substantially more than 10,000 primes, this way is just as fast, or faster. (You're not loading primes you won't need...)
    2. Unless you have a reason for only putting 10,000 primes in a file, I'd put everything in the same file. It makes working with them later a little easier.
    Of course, the question arises of why you're even doing it this way. If you already have the list of primes, why not just start with lowrange and go to the hirange of the primes. If you're trying to determine further primes, you're going to find non-primes because you're not increasing your list of primes. (What if your list of primes ends at the 10,000th prime, but your range includes the 10,001st prime squared ...) This way, you guarantee to find only primes. 
    #!/usr/local/bin/perl -w

use strict;

######################################################################
+##########

my $resultFile = "results.txt";

my $lowrange = 0;
my $hirange = 0;

######################################################################
+##########

print "Welcome to the prime number program.\n";
print "This program will tell you every prime number within a certain 
+range.\n";

print "Please enter the lower range now: ";
chomp($lowrange = <STDIN>);

die "\nThe low must be a positive integer\n"
        if ($lowrange && $lowrange =~ /\D/);

print "\nHow high should the program search for primes? ";
chomp($hirange = <STDIN>);

die "\nThe high must be a positive integer\n"
        if ($hirange && $hirange =~ /\D/);
die "\nThe high must be greater than or equal to the low\n"
        if ($hirange < $lowrange);

print "Now listing the range of all primes between $lowrange and $hira
+nge. \n";
print "Please wait.\n";

$lowrange++ if ($lowrange % 2 == 0);

open RESULTS, ">$resultFile"
        || die "Cannot open $resultFile for writing\n";

my $counter = 0;

if ($lowrange <= 3) {
        if ($lowrange <= 2) {
                print RESULTS "2\n";
                $counter++;
        }

        print RESULTS "3\n";
        $lowrange = 5;
        $counter++;
}

CANDIDATE:
for (my $candidate = $lowrange; $candidate <= $hirange; $candidate += 
+2) {

        for my $divisor (3 .. int(sqrt($candidate))) {
                next CANDIDATE
                        unless $candidate % $divisor;
        }


        $counter++;
        print RESULTS "$candidate\n";
}

close RESULTS;

print "\nTotal amount of primes: $counter \n";
print "Your results have been saved to $resultFile\n";

__END__

    [download]

    ------
    We are the carpenters and bricklayers of the Information Age.

    Vote paco for President!

[reply]
[d/l]
Re: Mock my code!
by Cine (Friar) on Sep 06, 2001 at 17:59 UTC
    I cleaned it up a bit... 
    !/usr/local/bin/perl -w
use strict;

#These variables are user-configurable.
my $countermax = 10000;          #The max number of primes per file. (
+To regulate file size)
my $primefile = "Primes.seed";    #The location of the seed file. This
+ is needed.
my $path = "./";     #The first part of the path to dump the files in.
+ This must be pre-existing.
my $last = ".txt";                #The extension to append to the file
+names. Your choice.
my $beginloc = 0;           #The number to start counting at when maki
+ng filenames.

#End users should not edit below this line
my $outputloc = $beginloc;

#First this looks for a pre-existing primes file
#which has all the primes less than 10000 in it. 
#It needs this file. Assume it is present or
#suggest a better way or something...
open PRIMES, $primefile || die $!;
my @parray = <PRIMES>;
close PRIMES;

open RESULTS, ">".$path.$outputloc.$last || die $!;

print <<TXT;
Welcome to the prime number program.
This program will tell you every prime number within a certain range.
Please enter the lower range now:
TXT

#get from arg or stdin
my $lowrange = int($ARGV[0]||<STDIN>);
chomp $lowrange;

#Simple idiot-proofing. I didn't bother to check to make
#sure it's actually an integer. What's a good way to 
#do this?
if($lowrange < 0) { 
   $lowrange = 0; 
   print "\nYour number must be positive. I have set it to 0 for you."
+;
}

print "\nHow high should the program search for primes? ";

#get from arg or stdin
my $hirange = int($ARGV[1]||<STDIN>);
chomp $hirange;

if($hirange <= $lowrange) { 
  $hirange = $lowrange + 100;
  print <<TXT;

Your second number must be at least equal to the first.
I have arbitratily set it to be 100 greater.
TXT
}


print "Now computing the range of all primes between $lowrange and $hi
+range. \nPlease wait.\n";

#No need to check even numbers, right?
$lowrange++ unless $lowrange % 2;

my $counter=0;
#Of course, since 1 isn't a prime, we have to account
#for that.
if($lowrange == 1) {
   print RESULTS "2\n";
   $lowrange = 3;
   $counter++;
}

for(my $current = $lowrange; $current <= $hirange; $current += 2) {
  for(my $i = 0; $parray[$i] <= sqrt($current); $i++) {
    if($current % $parray[$i] == 0) { 
      $counter++;
      print RESULTS "$current\n";
      if($counter%$countermax == 0) {
        close RESULTS;
        $outputloc++;
        print "\nFile full. Currently ".($current *100/$hirange)."\% d
+one.";

        open RESULTS, ">".$path.$outputloc.$last || die $!;
      }
      last;
     }
  }
}

print <<TXT;

Total amount of primes: $counter
Your results have been saved, starting with $path$beginloc$last

TXT

    [download]


    T I M T O W T D I
[reply]
[d/l]
Re: Mock my code!
by blakem (Monsignor) on Sep 07, 2001 at 12:54 UTC
    FWIW, the fastest all-perl prime generator that I've found for small primes (i.e. less than a billion or so) is in merlyn's primes article. If you are serious about performance, I'd recommend reading it carefully.

    Update: The link appears to be down at the moment, but I'm pretty sure its correct...
    For the impatient, here is a link to the article in google's cache (thanks rob_au)

    -Blake

[reply]
[reply]
Re: Mock my code!
by gornox_zx (Priest) on Sep 06, 2001 at 22:43 UTC
    It would improve the aestheticness as well as coding style, were you to use indentation to deliminate nested blocks of code. Such as loops and if statements.
    ~heise2k~
[reply]

Back to Seekers of Perl Wisdom