note
liverpole
Hi [orange],
<p>The main problem you're having is in your <c>is_prime</c> subroutine.
</p><p>You are calculating primes based on whether a number N is divisible by any value from 1 to N, when you only need to be looking at all odd values between 3 and sqrt(N), but including the special case of 2. Additionally, you should be caching the values (you may have thought you were doing this with <c>my @prime_;</c>, but the correct way would be to use a hash. (Additionally, though you were checking the value with <c>if ($prime_[1] == $num) { $b = 1; }</c>, you were still letting the entire loop run its course, and thus defeating its purpose).
</p><p><b>Update</b>: But see [grinder]'s important comment, below, on an even further degree of optimization.
</p><p><b>Update</b>: [bart] caught a bug in my program; namely, that it needs to test for division by 2! I've added this in the code below.
</p><p>Here's a rewrite of the entire program:
<c>
#!/usr/local/bin/perl
use strict;
use warnings;
use Tk;
my $NumOfPrimes = 0;
my ($colr, $counter);
my($o, $s) = (250, 20);
my($pi, $x, $y) = (3.1415926, 0, 0);
print "type the maximum number for the prime spiral \n";
my $lastnumber = <>;
print "please wait \n";
my $mw = MainWindow->new;
my $c = $mw->Canvas(-width => 500, -height => 500);
$c->pack;
$c->createLine(50, 250, 450, 250);
$c->createText(10, 250, -fill => 'blue', -text => 'X');
$c->createLine(250, 50, 250, 450);
$c->createText(250, 10, -fill => 'blue', -text => 'Y');
my $num = 0;
my $order = 0;
my $pnts = 0;
my $pntcycle = 0;
OUTER:
for (my $i= 1; $i <= 250; $i += 1) {
$order=$order+1;
if ($order == 5) {$order = 1;}
++$pntcycle;
if ($pntcycle == 3) {
$pntcycle = 1;
++$pnts;
}
$counter = 0;
while ($counter <= $pnts) {
$counter++;
++$num; # This is the more common way to increment a number
if ($num == $lastnumber + 1) { last OUTER }
my $b = is_prime($num);
if ($b != 0) {$colr = "white"; $NumOfPrimes = $NumOfPrimes + 1;}
if ($b == 0) {$colr = "black";}
if ($order == 1) {$x = $x + 1;}
elsif ($order == 2) {$y = $y - 1;}
elsif ($order == 3) {$x = $x - 1; }
elsif ($order == 4) {$y = $y + 1; }
$c->createText( $x+$o, $y+$o, -fill => "$colr", -text => '.');
}
}
MainLoop;
BEGIN {
# Cache primes for future use, but using a *hash*, NOT an array!
# Start with the value "2", which lets us avoid
my %primes = ( 2 => 1 );
sub is_prime {
my ($num) = @_;
if (exists($primes{$num})) {
return $primes{$num};
}
# Only need to calculate up to sqrt($num).
# This is VERY important, especially with larger numbers.
# If N is divisible by some factor $f, less than sqrt($N),
# then it's also divisible by some other factor $k = $N/$f
# which MUST be larger than sqrt($N), and vice versa.
my $sqrt = int(sqrt($num));
# Check for division by 2 (special-case)
($num % 2) or return 0;
# We've tried 2 as a factor, so any other factors must be odd
# (and prime, for that matter).
for (my $j = 3; $j <= $sqrt; $j += 2) {
if (0 == $num % $j) {
# It's NOT a prime.
return 0;
}
}
# Cache the prime number and return '1'
return $primes{$num} = 1;
}
}
</c>
</p><p>A couple of other points ...
<ol>
<li>You should use [doc://warnings], to get lots of valuable debugging information, like the fact that <c>$lastnumber</c> was declared twice.
<li><c>my ($order,$pntcycle,$pnts,$b,$num,$p) = (0);</c> likely does NOT do what you want. It's only setting the value of <c>$order</c> to zero. To set them all equal to zero, do something like <c>my $order = $my $pntcycle = my $pnts = ... = my $p = 0;</c>, or individually (the way I did above).
<li>It's not generally a good idea to make <i>everything</i> global, especially values into and out of a subroutine. You really should be passing <c>$num</c> to <c>is_prime()</c>, and returning the value which you assign to <c>$b</c>.
<li>You don't need to quote numbers when you put them into a hash -- <c>$prime_[$p] = "$j";</c> is better written <c>$prime_[$p] = $j</c>.
<li>It's more "Perl-like" to increment a variable with <c>++$var</c>. <c>$var = $var + 1</c> looks too much like, well, [wp://BASIC]. :-)
<li>Many will find your code easier to read if you indent it.
</ol>
<div class="pmsig"><div class="pmsig-465654">
<hr />
<font size="1">
s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
</font>
</div></div>
610218
610218