2
\$\begingroup\$

I'm a newbie in programming, just started about 6 months ago and just finding my way around in C# (clean) code. This is the toy task we have received from our teacher.

This is what it should do:

New article numbers are assigned in your company. They consist of 5 numbers and a check digit.

  • You are to design a program that reads in 5 single-digit numbers, calculates the check digit and outputs it to the console. It is up to you how you deal with incorrect entries.

  • You get the following information for the calculation of the check digit:

    • even numbers are added, odd numbers are multiplied by 3 and then added.
    • The check digit results from the last digit of the sum.
  • Example:

    27493 => 2+4 = 6;
    7*3+9*3+3*3 = 57;
    6 + 57 = 63 => check digit 3
    Item number = 27493-3

Since I'm a newbie, i dont have any own style at all and would love to hear all your suggetions. I have only heard about Clean Code, the book from Uncle Bob is already ordered. Maybe you have suggestions what I can look for first in such code, and of course what is common or not in a big business. I'm looking for a efficient and common way of writing code. I appreciate all your suggestions, even you find it obvious, i'm learning. Please suggest any do's and don'ts.

 public static void JugglingWithNumbers()
        {
            int[] numbers = new int[5];
            int[] evenNumber = new int [5];
            int[] oddNumber = new int [5];
            int sumEvenNumbers = 0;
            int sumOddNumbers = 0;
            int summeSusumme;

            Console.WriteLine( $" 5 numbers (1 digit) please:");

            for (int i = 0; i <5; i++)
            {
                bool ok;
               ok = int.TryParse(Console.ReadLine(), out numbers[i]);
                if (!ok)
                {
                    Console.WriteLine($"This is not a valid input");
                    Console.WriteLine($"PLease try again");

                    i--;
                }
            }
            for (int i = 0; i < numbers.Length; i++)
            {
                if (numbers[i]%2 == 0)
                {
                    evenNumber[i] = numbers[i];
                    sumEvenNumbers += evenNumber[i];
                }
                else
                {
                    oddNumber[i] = numbers[i];
                    sumOddNumbers += numbers[i] * 3; 
                }
            }
            Console.WriteLine($"Here are your numbers one more time - control output:");
            for (int i = 0; i < numbers.Length; i++)
            {
                Console.WriteLine(numbers[i]);
                
            }
            Console.WriteLine($"Even numbers");
            for (int i = 0; i < evenNumber.Length; i++)
            {
                Console.WriteLine(evenNumber[i]);

            }
            Console.WriteLine($"Odd Numbers");
            for (int i = 0; i < oddNumber.Length; i++)
            {
                Console.WriteLine(oddNumber[i]);

            }
            Console.WriteLine($"Sum even numbers: {sumEvenNumbers}");
            Console.WriteLine($"Sum odd numbers: {sumOddNumbers}");

            summeSusumme = sumEvenNumbers + sumOddNumbers;

            Console.WriteLine($"Your checksum: ");
            for (int i = 0; i < numbers.Length; i++)
            {
                Console.Write(numbers[i]);
            }
            Console.Write("-");
            Console.Write(summeSusumme%10);
        }
        static void Main(string[] args)
        {
            JugglingWithNumbers();
            Console.ReadLine();
        }
    }
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Please could you add a description of what the code should do? You're also missing a double quote after control output:. See how do I ask a good question \$\endgroup\$
    – RobH
    Apr 19, 2022 at 9:40
  • \$\begingroup\$ Done. Thanks for your feedback! \$\endgroup\$
    – miles
    Apr 19, 2022 at 9:51
  • 2
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Apr 19, 2022 at 12:34
  • 2
    \$\begingroup\$ This would be a good question if you change the title to reflect what the code is doing and move the question in the title into the body of the post. It would be even better if the main() was provided. \$\endgroup\$
    – pacmaninbw
    Apr 19, 2022 at 14:10
  • \$\begingroup\$ Thanks for all your hints guys. I try to implement them from now on so. \$\endgroup\$
    – miles
    Apr 20, 2022 at 8:39

1 Answer 1

3
\$\begingroup\$

You have a lot of very similar code repeated. This is a good hint that you can benefit from introducing a helper method to remove the duplication. For example, you have the need to output a list of numbers to the console multiple times. Start off by just extracting your code:

private static void PrintNumbers(string heading, int[] numbers)
{
    Console.WriteLine(heading);
    for (int i = 0; i < numbers.Length; i++)
    {
        Console.WriteLine(numbers[i]);    
    }
}

You can now change the printing in the middle of your code to something much easier to read:

PrintNumbers("Here are your numbers one more time - control output:", numbers);
PrintNumbers("Even numbers", evenNumber);
PrintNumbers("Odd Numbers", oddNumber);

As well as being easier to read, you can now be sure that all the output is consistent.

It's worth now refactoring the implementation of PrintNumbers:

private static void PrintNumbers(string heading, IEnumerable<int> numbers)
{
    Console.WriteLine(heading);
    Console.WriteLine(string.Join(Environment.NewLine, numbers));
}

This uses string.Join instead of multiple calls to Console.WriteLine, the input also uses the most general type* we can. * well, without adding generics. Learn to love string.Join - it's incredibly useful.

You've used fixed size datastructures (array) for each of the numbers, oddNumber and evenNumber variables. However, you know that oddNumber and evenNumber can't both be full - a List<int> is a much better choice for those:

var oddNumbers = new List<int>();
var evenNumbers = new List<int>();
int evenSum, oddSum;

foreach (var n in numbers)
{
    if (n % 2 == 0) 
    {
        evenNumbers.Add(n);
        evenSum += n;
    }
    else
    {
        oddNumbers.Add(n);
        oddSum += n * 3;
    }
}

Notice that we can now use a foreach loop and not worry about indexes at all.

If you don't want to output all numbers/evens/odds and intermediate sums, you can make the code much more concise:

public static void JugglingWithNumbers()
{
    var numbers = GetUserInput();
    var checkDigit = CalculateCheckDigit(numbers);
    Console.WriteLine($"Your checksum: {string.Join("", numbers)}-{checkDigit}");
}

private static int CalculateCheckDigit(IEnumerable<int> numbers)
{
    var sum = numbers
        .Select(n => n % 2 == 0 ? n : n * 3)
        .Sum();
    return sum % 10;
}

private static IEnumerable<int> GetUserInput()
{
    // Left as exercise
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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