14
\$\begingroup\$

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.
    }
}
\$\endgroup\$
18
\$\begingroup\$
  • 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

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Ben Rivers Mar 28 '18 at 12:30
  • 3
    \$\begingroup\$ @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#. \$\endgroup\$ – Mark Yisri Mar 28 '18 at 14:16
  • \$\begingroup\$ @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. \$\endgroup\$ – David Richerby Mar 29 '18 at 10:20
  • \$\begingroup\$ 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. \$\endgroup\$ – Woodrow Barlow Mar 29 '18 at 14:25
11
\$\begingroup\$

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;
            }...
\$\endgroup\$
11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Ben Rivers Mar 28 '18 at 16:04
3
\$\begingroup\$

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;
}
\$\endgroup\$
1
\$\begingroup\$
        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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.