Hi
orange,
The main problem you're having is in your is_prime subroutine.
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 my @prime_;, but the correct way would be to use a hash. (Additionally, though you were checking the value with if ($prime_[1] == $num) { $b = 1; }, you were still letting the entire loop run its course, and thus defeating its purpose).
Update: But see grinder's important comment, below, on an even further degree of optimization.
Update: 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.
Here's a rewrite of the entire program:
#!/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 n
+umber
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;
}
}
A couple of other points ...
- You should use warnings, to get lots of valuable debugging information, like the fact that $lastnumber was declared twice.
- my ($order,$pntcycle,$pnts,$b,$num,$p) = (0); likely does NOT do what you want. It's only setting the value of $order to zero. To set them all equal to zero, do something like my $order = $my $pntcycle = my $pnts = ... = my $p = 0;, or individually (the way I did above).
- It's not generally a good idea to make everything global, especially values into and out of a subroutine. You really should be passing $num to is_prime(), and returning the value which you assign to $b.
- You don't need to quote numbers when you put them into a hash -- $prime_[$p] = "$j"; is better written $prime_[$p] = $j.
- It's more "Perl-like" to increment a variable with ++$var. $var = $var + 1 looks too much like, well, BASIC. :-)
- Many will find your code easier to read if you indent it.
s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
-
Are you posting in the right place? Check out Where do I post X? to know for sure.
-
Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
<code> <a> <b> <big>
<blockquote> <br /> <dd>
<dl> <dt> <em> <font>
<h1> <h2> <h3> <h4>
<h5> <h6> <hr /> <i>
<li> <nbsp> <ol> <p>
<small> <strike> <strong>
<sub> <sup> <table>
<td> <th> <tr> <tt>
<u> <ul>
-
Snippets of code should be wrapped in
<code> tags not
<pre> tags. In fact, <pre>
tags should generally be avoided. If they must
be used, extreme care should be
taken to ensure that their contents do not
have long lines (<70 chars), in order to prevent
horizontal scrolling (and possible janitor
intervention).
-
Want more info? How to link
or How to display code and escape characters
are good places to start.