2
\$\begingroup\$

Taking a problem from the website, "Project Euler #1", I have created my first C# program. The problem I have used is as follows:

If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

Find the sum of all the multiples of 3 or 5 below 1000.

The program I have made:

using System;

namespace _5_multiples
{
    public class MainClass
    {
        public static void Main()
        {
            int currentNumber = 1000;
            int allSum = 0;

            currentNumber = currentNumber - 1;
            while (currentNumber > 0)
            {
                if (currentNumber % 3 == 0)
                {
                    allSum = allSum + currentNumber;
                }
                else
                {
                    if (currentNumber % 5 == 0)
                    {
                        allSum = allSum + currentNumber;
                    }
                }
                currentNumber = currentNumber - 1;
            }
            Console.WriteLine(allSum);
        }
    }
}

I understand there is probably a lot to improve - this is my first C# program after all. I did run into a few problems such as referencing non static members from a static context and no suitable entry method. Rather than finding a definitive fix, the solution I implemented was more of a workaround; hence why I feel there may be a lot to improve.

namespace _5_multiples - this doesn't look right? Name of the program is actually 3_5_multiples - perhaps a badly chosen name?

Anyway, please let me know on anything that could be improved.

\$\endgroup\$
  • \$\begingroup\$ You should use the for loop. \$\endgroup\$ – Denis Jul 13 '17 at 13:04
  • 1
    \$\begingroup\$ It is a good practice to leave a question open for a day or two to get a full range of answers. \$\endgroup\$ – paparazzo Jul 13 '17 at 14:21
5
\$\begingroup\$

Here's what I would change:

  • I'd use a for loop instead of a while loop here: for (int currentNumber = 999; currentNumber > 0; currentNumber--). This keeps all loop-related things grouped together. Loop bodies can be skipped or broken out of, so if code that must be executed every cycle is put at the end of a loop body it's easily broken when the loop body grows more complex and someone adds a continue or break in there.
  • Actually, there is no need to count down - the result will be the same either way. Counting up is often a little easier to understand: for (int currentNumber = 0; currentNumber < 1000; currentNumber++). Note that using < here means you don't need to decrement currentNumber by one before the loop starts.
  • You can write a += b instead of a = a + b (the same goes for various other operators).
  • else { if { .. } } can be simplified to else if { ... }.
  • Though in this case, a single if statement is sufficient: if (currentNumber % 3 == 0 || currentNumber % 5 == 0) { ... }. The 'or' operator (||) short-circuits if the first expression is true, so if a number can be divided by 3 it won't also try to divide it by 5.
  • C# naming conventions typically write class, method and namespace names in PascalCase.

Regarding your questions:

  • Static things belong to a class, non-static things belong to instances of that class. Classes are to instances what blueprints are to actual buildings. Trying to access a non-static (instance) members from a static context is like trying to open a door on a blueprint. The blueprint doesn't contain any real doors, only actual buildings do.
  • Programs need a place to start executing, and in C# the starting point is a static Main method. Putting everything in there is fine for a small program like this.
  • Identifier names in C# cannot start with a number, so 3_5_multiples isn't a valid name. I'd probably use EulerProblem1 or something like that here.
  • Namespaces are useful for organizing larger programs (namespaces to classes are a bit like folders to files). The root namespace of a project is usually the name of that project itself, and child namespaces often correspond to folders.

EDIT: Part of the fun on Project Euler is coming up with more efficient solutions. Can you find a way to solve this without checking every number below 1000?

\$\endgroup\$
0
\$\begingroup\$

I have edited my program following the advice given on this thread:

using System;

namespace EulerProblemOne
{
    public class MainClass
    {
        public static void Main()
        {
            int allSum = 0;
            Console.WriteLine("Enter a number...");
            int inputNumber = Convert.ToInt32(Console.ReadLine());
            inputNumber -= 1;

            for (int currentNumber = inputNumber; currentNumber > 0; currentNumber--)
                if ((currentNumber % 3 == 0) || (currentNumber % 5 == 0))
                {
                    allSum += currentNumber;
                }
            Console.WriteLine(allSum); 
        }
    }
}

I now have the following:

  • Renamed the program so that it no longer starts with a number.
  • The number to check under can now be inputted by the user and not a fixed literal.
  • A for loop is used instead of a while loop.
  • IF statements have been combined into a single IF statement using OR.
  • Arithmetic lines now use add/subtract and assign in a single operator.
\$\endgroup\$
  • \$\begingroup\$ You could also have loop go forward as in for (int currentNumber = 1; currentNumber <= inputNumber; currentNumber++). The conditional could be changed to < if you did not issue inputNumber -= 1. \$\endgroup\$ – Rick Davin Jul 13 '17 at 14:20
  • 1
    \$\begingroup\$ You should always put the logic of a for loop in a code block (i.e. surround it with { and }. This will prevent you from very nasty bugs. Image what would happen if you put a Console.WriteLine() on the line before your if-statement? \$\endgroup\$ – venerik Jul 13 '17 at 14:29
0
\$\begingroup\$

You can just use the loop counter for currentNumber
Can use an or for divisible

public static int Numbers(int toNumber)
{
    int allSum = 0;
    for (int i = 3; i < toNumber; i++)
    {
        if (i % 3 == 0 | i % 5 == 0)
        {
            allSum += i;
        }
    }
    Debug.WriteLine(allSum);
    return allSum;
}
\$\endgroup\$
-2
\$\begingroup\$
    using System;

namespace _5_multiples
{
    public class MainClass
    {
        public static void Main()
        {
            int currentNumber = 1000;
            int allSum = 0;

            currentNumber = currentNumber - 1;
            while (currentNumber > 0)
            {
                if ( (currentNumber % 3 == 0) | (currentNumber % 5 == 0))
                {
                    allSum = allSum + currentNumber;
                }
                currentNumber = currentNumber - 1;
            }
            Console.WriteLine(allSum);
        }
    }
}

This is my advice, you didn't need both -if-, only one is necessary.

\$\endgroup\$
  • \$\begingroup\$ | is a binary operator, || is the boolean one, which should be used here. And are you sure your second example is working properly (apart from the compile errors)? Some numbers can be divided by both 3 and 5. \$\endgroup\$ – Pieter Witvoet Jul 13 '17 at 12:33
  • \$\begingroup\$ It was more about giving the OP an idea of improvement than giving you a perfect answer. But yeah you're right about the fact that some are both multiples of 5 and 3 I'll edit my post :). For the question of | or || we don't really care since we are comparing ones or zeros. \$\endgroup\$ – YCN- Jul 13 '17 at 12:37
  • \$\begingroup\$ Binary & instead of boolean &&, more variable name typo's, currentNumber5 is still unassigned before use, and since currentNumber3 and currentNumber5 go down at different 'speeds' they won't be equal, so the result still isn't correct. Personally, I don't mind, but the OP is new to C#, so showing them broken and incorrect code isn't very helpful. \$\endgroup\$ – Pieter Witvoet Jul 13 '17 at 12:45
  • 1
    \$\begingroup\$ Tip: if creating a project in Visual Studio is too much effort, then tools like Linqpad can be useful to quickly verify snippets of code like this. \$\endgroup\$ – Pieter Witvoet Jul 13 '17 at 12:46
  • 2
    \$\begingroup\$ I really tried to understand your second approach but failed. After fixing the syntax errors and running it on ideone it produced a wrong result. \$\endgroup\$ – Martin R Jul 13 '17 at 13:36

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.