6
\$\begingroup\$

Started learning C# second time. I have written a console program to calculate sum and count of even and odd numbers. Provided code :

using System;

namespace Studying_
{
    class Program
    {
        static void Main(string[] args)
        {
            uint evenNumbersCount = 0;
            uint oddNumbersCount = 0;
            int sum = 0;

            Console.WriteLine("Enter the first number in the range to find out it odd or even. The program won't count numbers you entered in the sum");
            int numberOne = int.Parse(Console.ReadLine());

            Console.WriteLine("Enter the second number");
            int numberTwo = int.Parse(Console.ReadLine());

            if (numberTwo < numberOne)
            {
                int a = numberTwo;
                numberTwo = numberOne;
                numberOne = a;
            }

            while(numberOne < numberTwo - 1)
            {
                numberOne++;

                int result = numberOne % 2;
                sum += numberOne;

                switch (result)
                {
                    case 0:
                        evenNumbersCount++;
                        break;
                    case 1:
                        oddNumbersCount++;
                        break;
                }
            }

            Console.WriteLine("Sum - " + sum + " | Even - " + evenNumbersCount + " | Odd - " + oddNumbersCount);

            Console.ReadKey();
        }
    }
}

Input : 2, 5
Output : "Sum - 7 | Even - 1 | Odd - 1
2 and 5 aren't counted

Is this code good? Can I improve it or optimize?

P.S. Sorry for my english and if I did something off-topic. Let me know if something is wrong.

\$\endgroup\$
8
  • \$\begingroup\$ Have you tested the results? The first number will not be counted as you increment it before summing. The second one will be excluded as well. Shouldn't the range be inclusive? \$\endgroup\$ Jun 7, 2021 at 13:52
  • \$\begingroup\$ Yes, I did, it works as it supposed. I know how to make it counted if I need it. \$\endgroup\$
    – JediMan
    Jun 7, 2021 at 13:59
  • 1
    \$\begingroup\$ Just a small thing. Your main routine prompts the user with the string, "Enter the first number in the range to find out it odd or even..." That isn't bad and because I can see your code I know what are allowable values. I'd like to suggest you provide the range, something like, "Enter an integer value from -2147483648 to 2147483647". Or restrict it for the user to something like "-10000 to 10000". Its likely most people would think of integer values, rather than trying to enter something like 3.14159. But, just to prompt them is good. \$\endgroup\$
    – Rod
    Jun 7, 2021 at 23:46
  • \$\begingroup\$ Are you interested in simplifications like just counting odd numbers, and then calculating even count as evens = n - odds where n is the length of the range? Obviously the whole thing here could be reduced to closed-form so you don't need to loop. (Odd and even are both about n/2, with adjustments based on the first and last numbers. And Gauss's n * (n+1)/2 for the sum of 1..n can be adapted for a range, and also done without possible overflow if necessary, like clang does when optimizing a sum+=i loop like this. But I'm guessing that these are placeholders for real work.) \$\endgroup\$ Jun 8, 2021 at 1:20
  • 1
    \$\begingroup\$ I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Heslacher
    Jun 8, 2021 at 8:06

4 Answers 4

9
\$\begingroup\$

The naming could be better. numberOne, numberTwo and result does not tell anything about what these numbers represent. Better rangeStartExclusive and rangeEndExclusive. The ...Exclusive makes it clear that the bounds will not be counted.

I also would use a for-loop. This is the standard way of looping a number range. It also allows us to easily define a loop variable. The problem of incrementing numberOne is that it makes it a range bound and a running variable at the same time. This double role decreases readability.

Instead of an int result, I would use a Boolean telling its meaning.

for (int n = rangeStartExclusive + 1; n < rangeEndExclusive; n++) {
    bool isEven = n % 2 == 0;
    if (isEven) {
        evenNumbersCount++;
    } else {
        oddNumbersCount++;
    }
    sum += n;
}

Alternatively, you could keep your original implementation and instead rename result to remainder.

It is okay to give our number a non-descriptive name like i or n, often used in math to denote a whole number. i is often used for indexes.

To keep the declaration and use of isEven close together, I have moved sum += n to the end of the loop. It looks strange if you first calculate result, then calculate an unrelated sum and only then use result.

Why use uint for the count of even and odd numbers? This count will always be smaller than the range bounds given as int. The maximum uint is about the double of the maximum int.

However, the sum could exceed the maximum int. The sum of all whole numbers starting at 1 and up to n is n * (n + 1) / 2. When starting counting at 1, this limits the maximum number at 65,535. Therefore, it would make sense to use long for the sum.

\$\endgroup\$
5
  • \$\begingroup\$ Is there some advantages to use if instead of switch? In my opinion, switch is more readeable here. Or is it so because switch requires more performance? \$\endgroup\$
    – JediMan
    Jun 7, 2021 at 14:31
  • \$\begingroup\$ The main reason was to be able to give the variable result the name isEven. You could also name it remainder instead and keep the original code. \$\endgroup\$ Jun 7, 2021 at 14:39
  • 2
    \$\begingroup\$ In traditional / pseudo-code 1-letter var names, n is most often used for loop upper bounds, with i, j, or k as a counter. It's not totally weird to use n as the loop counter / induction variable, but it's more common and idiomatic IMO for n to be loop invariant. \$\endgroup\$ Jun 8, 2021 at 1:06
  • 1
    \$\begingroup\$ Another advantage of n % 2 == 0 is that this way works for negative odd numbers. @JediMan's switch would only count positive odd numbers, because -5 % 2 == -1 not +1. Probably a good idea to mention that correctness reason along with stylistic reasons in this answer. \$\endgroup\$ Jun 8, 2021 at 1:11
  • \$\begingroup\$ @PeterCordes, The reason I have chosen n is that while it is used as loop variable, this is not its primary intent. We want to perform a mathematical analysis (even/odd) on this number. For this purpose n is good. Maybe a more consequent approach would be to use i as loop variable and to pass it over to a method doing the math part as a parameter named n. \$\endgroup\$ Jun 8, 2021 at 12:42
8
\$\begingroup\$

You already have answers, but lets add one more for learning purpose. This is another way of doing the same thing. Not a better way, just an alternative one to showcase some C# features. If you feel a bit overwhelm, it is normal. You don't have to know everything and nobody expect you to do. If you have any questions, feel free to post them in comment. I will be more than happy to answer them :)

We will use tuple, tuple's deconstruction, linq, interpolated string, lookup, lambda operator and more. Take the time to read the docs above, there are a lot of things going on below. Here we go:

using System;
using System.Collections.Generic;
using System.Linq;
                    
public class Program
{
    public static (int, int) SumCount(IEnumerable<int> source) => (source.Sum(), source.Count());

    public static (int, int, int) SumCount2(IEnumerable<int> source, Func<int, bool> predicate)
    {
        var results = source.ToLookup(predicate);
        var (evenSum, evenCount) = SumCount(results[true]);
        var (oddSum, oddCount) = SumCount(results[false]);
        return (evenSum + oddSum, evenCount, oddCount);
    }

    public static IEnumerable<int> RangeWithoutStart(int start, int end) => Enumerable.Range(start + 1, end - start - 1);

    public static void Main()
    {
        var start = 2;
        var end = 6;
        var range = RangeWithoutStart(start, end);
        var (sum, evenCount, oddCount) = SumCount2(range, x => x % 2 == 0);
        Console.WriteLine($"Sum - { sum } | Even - { evenCount } | Odd - {oddCount}");
    }
}

Try it Online

The current implementation of SumCount will loop twice into source, we can reduce to one by using Aggregate:

public static (int, int) SumCount(IEnumerable<int> source) => source.Aggregate((0, 0), (a, b) => (a.Item1 + b, a.Item2 + 1));

Learning concept:

These concepts are keys in Functional Programming but here it shows the beauty of a multi-paradigm language like C#.

\$\endgroup\$
5
  • \$\begingroup\$ I think this answer is too advanced for the beginner level of the OP. \$\endgroup\$
    – Rick Davin
    Jun 7, 2021 at 15:45
  • 3
    \$\begingroup\$ @RickDavin I think it is nice to discover new way of doing stuff. I learnt linq by looking at example. I decided to post because I dont think that I am the only person who love to learn by example. I will add a caveat though. \$\endgroup\$
    – aloisdg
    Jun 7, 2021 at 15:49
  • 1
    \$\begingroup\$ Thank you very much! This code is a set of symbols and words for me, so it definetily on another level for now) Currently, I'm studying with a course on youtube, but after the end, I promise to have a deal with it. \$\endgroup\$
    – JediMan
    Jun 7, 2021 at 18:41
  • \$\begingroup\$ @JediMan You will have all the time needed. Have fun learning. C# is a great language. \$\endgroup\$
    – aloisdg
    Jun 7, 2021 at 19:05
  • \$\begingroup\$ @RickDavin Learning new things is hard. It gets easier and more relevant when the thing solves an active problem you have (under the same vein as "even food you don't like tastes better when hungrier"). OP is in a prime position to understand and learn, as they are acutely aware of the problem domain since they just tried to solve it themselves. Code reviews can entail improved implementations when it adds something (and is not just an arbitrary rewrite), as is the case here. \$\endgroup\$
    – Flater
    May 18 at 17:10
6
\$\begingroup\$

You should try your program whith different values. When asked for the first number, enter "abcdef". Hit enter, and see what happens.

In such a case, when you can't be sure that the string to parse is ok, use int.TryParse(...).

\$\endgroup\$
4
  • \$\begingroup\$ Yes, thanks. I know this program will throw exception if I enter "abcdef". The question is whether I can improve code somewhere to gain more performance or make better for understanding. Sorry for misunderstanding. \$\endgroup\$
    – JediMan
    Jun 7, 2021 at 14:00
  • 2
    \$\begingroup\$ Will it be better to use try catch instead of TryParse? If I use TryParse(), it will return zero and my calculating will be wrong. Isn't it good to use try catch to catch exception and do a warning then? \$\endgroup\$
    – JediMan
    Jun 7, 2021 at 14:02
  • 2
    \$\begingroup\$ @JediMan Good question. Try it with TryParse and you'll find it actually returns false! TryParse returns true if it is able to parse, and false otherwise. That means you can use it in an if block and end the program or display an error if the user entered bad input. \$\endgroup\$ Jun 8, 2021 at 3:58
  • \$\begingroup\$ @Nate Barbettini, understood and dealed with TryParse(), thank you. \$\endgroup\$
    – JediMan
    Jun 8, 2021 at 8:02
0
\$\begingroup\$

Sure it's good. I think loops in this case are not needed. The following is based on the famous:

1 = 1^2
1+3 = 2^2
1+3+5 = 3^2
...

Included the loop so you see how it waits. It is not well tested and it can be improved:

namespace odd
{
    public record Result
    (
        long oddCount,
        long eavenCount,
        long sum
    );
    class Program
    {
        static Result mathPreliminaries(long start, long finish){
            // remove one from finish and add one to start
            // to make them exclusive
            // you can also hack other parts to get desired output
            finish -= 1;
            start += 1;
            // total numbers in this range 
            // eg start = 2, finish = 5 => [2, 3, 4, 5] => count = 5 -2 + 1 = 4
            long totalNumCount = finish - start +1;
            // the following is the same as {finish//2 + finish%2} just(i think) more efficient with bit hack
            // you can see by {1+ 3+ 5 = 3 ^2}  that the total count of odds is 3, same as:
            // 5//2 = 2 + 5%2 , that is 3
            long finishOddCount = (finish >> 1) + (finish & 1);
            // we do the same for the starting number
            // from previous example would be starting at number 2: 
            // 2//2 = 1 + 2%2 = 1, so you see there is only one odd {1 = 1^2}
            long startOddCount = (start >> 1) +(start & 1);
            // now you apply the square part to get the sum 
            // {1+ 3+ 5 = 3 ^2 = 9}
            long oddFinishSum = finishOddCount * finishOddCount;
            // {1 = 1^2 =  1}
            long oddStartSum =  startOddCount * startOddCount;
            // the sum  of odds would be 
            // the sum of odds to the finish minus the the sum of odds to start
            // in this case 9 - 1 = 8 and if the starting number is odd we would sum it ass well
            // since 0b10 & 1 = 0 * 2 = 0 in this example is not sumed
            long oddSum = oddFinishSum - oddStartSum + ((start & 1) * start);
            // the odd count is done in the same fashion
            // 3 - 1 = 2 and we would sum one if start was odd
            long oddCount = finishOddCount - startOddCount + (start & 1);
            // now this I had to write a bit on white board but is quite simple to see
            // 1 + 3 + 5 = 9  and 2 + 4 = 6
            // if finish was even that would be 2 + 4 + 6 = 12
            // 6 = 9 - 3 that is sum of odd - count of odd is sum of even
            // if finish was even 9 + 3 = 12
            // in case you ar wondering the following is a ternary if else statement
            // you also see them in javascript
            // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/conditional-operator
            long evenFinishSum = (finish & 1) == 1 ? oddFinishSum - finishOddCount : oddFinishSum + finishOddCount;
            // same for the start 
            // 2 & 1 = 1 so 
            // odd start sum = 1 - 1 = 0
            long evenStartSum = (start & 1) == 1 ? oddStartSum - startOddCount : oddStartSum + startOddCount - start;
            // even sum is difference between the two
            // in this cas is 6- 0 = 6
            long evenSum = evenFinishSum - evenStartSum;
            // total sum is the sum of evens and odds
   
            long totalSum = oddSum + evenSum;
            // cound of evens is the total count minus the cound of odds
            return new Result(oddCount, totalNumCount - oddCount, totalSum);
        }


        public static void Main(){
            Console.WriteLine("Give me a Natural(including 0): ");
            // use long so you see time difference on big numbers
            long numberOne = Convert.ToInt64(Console.ReadLine());
            Console.WriteLine("Give me another: ");
            long numberTwo = Convert.ToInt64(Console.ReadLine());
            // if numbers are equal we return 0 as in the loop
            var result = new Result(0, 0 ,0);
            if (numberOne != numberTwo){
                // get the max number between the two
                long finish = Math.Max(numberOne, numberTwo);
                // not using min, just cancel the one that was larger
                long start = numberOne + numberTwo - finish;
                // get result from function
                result  = mathPreliminaries(start, finish);
            }
            Console.WriteLine($"Sum - {result.sum}, Even - {result.eavenCount}, Odd -{result.oddCount}");
            // run as loop to see time difference on large numbers
            long sum = 0;
            for (long i = numberOne+1; i < numberTwo; i++)
            {
                sum += i;
            }
            Console.WriteLine(sum);
        }   
    }
}
   

   

You can also mix this with: sum of n = ((n^2)/2) + n/2

\$\endgroup\$
1
  • 3
    \$\begingroup\$ This answer could use more insightful observations about the code. \$\endgroup\$
    – pacmaninbw
    May 16 at 21:25

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.