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.
    }
}

Answer 1

• 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

Answer 2

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;
            }...

Answer 3

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.

Answer 4

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;
}

Answer 5

        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.