Nearly Illegible Prime Number Calculator

Question

This is my first reasonably large C# program. Considering my code looks like it was written in an esolang to me, I'm guessing I did something wrong at some point. My main priority is readability, although speed is also a plus.

using System; public class PrimeSearcher { public static string Stringilate (int[] iterable) { string returnable=""; foreach(int i in iterable){ returnable+=i.ToString()+" "; }; return returnable; } public static bool isPrime (int[] iterable,float target) { foreach(int i in iterable){ if(target%i==0){ return false; } if(i==0){ return true; } //reached the "end" (last discovered prime) of the array, hurray! } return true; //will only do anything for the last element } static public void Main () { int[] primes=new int[1000]; float j=2; //float because I think it's the smallest datatype that can return a non-zero number when divided, correct me if I'm wrong while(primes[primes.Length-1]==0){ if(isPrime(primes,j)){ primes[Array.IndexOf(primes,0)]=(int)j; } j++; } Console.WriteLine(Stringilate(primes)); //I don't *think* ToString worked when I tried it. } }

Heslacher · Accepted Answer · 2018-03-28 09:34:26Z

By starting with j = 3 and incrementing j by 2 you could skip a lot of unneeded computations because it will skip even values.
Instead of using a float I would go with int. Both are 32-Bit and you won't need the floating point values.

You should let your variables have some space to breathe.

Consider

while(primes[primes.Length-1]==0){ if(isPrime(primes,j)){ primes[Array.IndexOf(primes,0)]=(int)j; } j++; }

versus

while (primes[primes.Length - 1] == 0) { if (isPrime(primes, j)) { primes[Array.IndexOf(primes, 0)] = (int)j; } j += 2; }

where the later is much more readable. It uses the bracing style most C# developer use as well.

Stringilate() could be improved as well by using string.Join() which would look like so
```
public static string Stringilate(int[] iterable) { return string.Join(" ", iterable); } 
```
and as a side note, if you need to concatenate strings in a loop its much better to use a StringBuilder instead.
You should be consitent in your coding style (public static vs. static public).
You are using braces although they might be optional. Good choice !
Instead of a concrete type you could use var instead

Thanks so much! I really need to familiarize myself with C# more. Concerning the brackets, I'm used to foo(){ because of my experience with other languages, but I'll keep that in mind when I need to write human-readable code. — Ben Rivers
– Ben Rivers, Commented Mar 28, 2018 at 12:30
@Ben Keep in mind that you can always use an indentation tool to reformat your code. That way you could theoretically write it in your style and automatically change it before publishing, provided that the indentation tool can handle changing your style to the one you pass to it. One such tool is GNU indent, but I'm not sure if it supports C#. — Mark Yisri
– Mark Yisri, Commented Mar 28, 2018 at 14:16
@BenRivers "human-readable code" Is there some other kind? The compiler doesn't care about whitespace but any code you write is going to be read by a human at some point. If you're unlucky, that human will be you. — David Richerby
– David Richerby, Commented Mar 29, 2018 at 10:20
i think it's worth explaining that spacing style a little bit: spaces go between keywords (if, while, etc.) and opening parentheses, but not between identifiers (function names) and opening parentheses. the first time i encountered this spacing style, nobody explained that rule and i was confused about where to add spaces versus not. — Woodrow Barlow
– Woodrow Barlow, Commented Mar 29, 2018 at 14:25

score 11 · Accepted Answer · 2018-03-28 11:10:25Z

This iteration is rather strange and inefficient:

int[] primes=new int[1000]; float j=2; //float because I think it's the smallest datatype that can return a non-zero number when divided, correct me if I'm wrong while(primes[primes.Length-1]==0){ if(isPrime(primes,j)){ primes[Array.IndexOf(primes,0)]=(int)j; } j++; }

Instead a more straightforward approach could be:

int count = 1000; int[] primes = new int[count]; primes[0] = 2; float j = 3; for (int i = 1; i < count; i++) { if (isPrime(primes, j)) { primes[i] = (int)j; } j += 2; }

In isPrime() it is sufficient to iterate up to and including the sqrt of the target:

foreach(int i in iterable.TakeWhile(x => x != 0 && x <= Math.Sqrt(target))){ if(target%i==0){ return false; }...

recursive · Accepted Answer · 2018-03-28 15:11:29Z

I would change the name of iterable. The fact that it's iterable is type information, that's already recorded in its actual type declaration, int[]. Changing its name to divisors makes it more clear what its purpose is.

This is a good point; vote++;. I'm used to weakly-typed languages like python, so I'm not used to some of these conventions. — Ben Rivers
– Ben Rivers, Commented Mar 28, 2018 at 16:04

paparazzo · Accepted Answer · 2018-03-29 14:17:18Z

This is not very efficient as string is immutable.

public static string Stringilate (int[] iterable) { string returnable=""; foreach(int i in iterable){ returnable+=i.ToString()+" "; }; return returnable; }

Why do they need to pass in iterable? You should use the value you have.

return string.join(" ", iterable);

main in that class does NOT work in my environment. Is that supposed to be the ctor? Not sure that is working code.

As a class it does not make sense. You don't even expose the result from the class.

Without hitting on every point this is how I would approach this:

test

int[] primes = PrimeSearcher(1000); Debug.WriteLine(string.Join(", ", primes));

code

private static bool isPrime(int[] primes, int target) { foreach (int i in primes.Where(x => x != 0)) { if (target % i == 0) { return false; } if (i * i > target) { return true; } } return true; } public static int[] PrimeSearcher(int count) { if (count < 1) { throw new ArgumentOutOfRangeException(); } int[] primes = new int[count]; primes[0] = 2; int j = 3; int index = 1; while (index < count) { if (isPrime(primes, j)) { primes[index] = j; index++; } j += 2; } return primes; }

Peter Taylor · Accepted Answer · 2018-03-29 10:32:32Z

 foreach(int i in iterable){ if(target%i==0){ return false; } if(i==0){ return true; } //reached the "end" (last discovered prime) of the array, hurray! } ... float j=2; //float because I think it's the smallest datatype that can return a non-zero number when divided, correct me if I'm wrong

My first instinct when I saw the loop was that the code must be buggy because if (i==0) then target%i would throw an exception. Then I figured it out: because target is a float, target%i gives NaN (which does not comparable equal to 0).

That is, I think, the only thing which depends on target being a float, since you're not actually using division anywhere. So the lack of comment in the loop (// target is a float, so division by 0 won't throw an exception) and the comment on j are both misleading.

Of course, the best fix is to ditch the floats and work in integers.

Stack Exchange Network

Nearly Illegible Prime Number Calculator

5 Answers 5

You must log in to answer this question.

Hot Network Questions

Nearly Illegible Prime Number Calculator

5 Answers 5

You must log in to answer this question.

Related

Hot Network Questions