6
\$\begingroup\$

This simple console program prompts the users for a number and returns the lowest and highest numbers from their inputs. It is difficult to add more user inputs, though. How can I make this easier to maintain and extend?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            int num1 = 0, num2 = 0, num3 = 0;

            Console.WriteLine("lowest");

            Console.WriteLine("input 1st number");
            num1 = int.Parse(Console.ReadLine());

            Console.WriteLine("input 2nd number");
            num2 = int.Parse(Console.ReadLine());

            Console.WriteLine("input 3rd number");
            num3 = int.Parse(Console.ReadLine());

           Console.WriteLine("input 4th number");
            num4 = int.Parse(Console.ReadLine());

               Console.WriteLine("input 5th number");
            num5 = int.Parse(Console.ReadLine());

            Console.Clear();



            int lowest = ((num1 < num2 ? num1: num2) < num3 ? 
                (num1 < num2 ? num1 : num2) : num3);
            int highest = ((num1 > num2 ? num1: num2) > num3 ? 
                (num1 > num2 ?  num1 : num2)  : num3) ;

            Console.WriteLine(" lowest is {0} ", lowest);
            Console.WriteLine(" highest is {0} ", highest);
            Console.ReadLine();
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Your code is broken: The variables num4 and num5 are read in but never used (or declared for that matter) in the lowest/highest calculation. \$\endgroup\$ Commented Jul 12, 2014 at 19:54

3 Answers 3

4
\$\begingroup\$

If you are only worried about showing Min and Max, and nothing else is the desired result then why not use an immediate calculation approach like this? It surely does not store unnecessary inputs -

using System;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            int max = Int32.MinValue, min = Int32.MaxValue;
            int value = 0;

            while (int.TryParse(Console.ReadLine(), out value))
            {
                max = Math.Max(max, value);
                min = Math.Min(min, value);

                Console.WriteLine(" lowest is {0} ", min);
                Console.WriteLine(" highest is {0} ", max);
            }
        }
    }
}

This program will print min and max, until a non-digit character is given.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Is there a specific reason you're not using Math.Max and Math.Min? \$\endgroup\$ Commented Jul 12, 2014 at 19:47
  • \$\begingroup\$ Nope. You can use Math.Max. I just try to avoid type casting as a whole. Ternary is faster than that. \$\endgroup\$ Commented Jul 12, 2014 at 19:50
  • 1
    \$\begingroup\$ There are int overloads for it, where does the type casting occur? msdn.microsoft.com/en-us/library/system.math.max(v=vs.110).aspx \$\endgroup\$ Commented Jul 12, 2014 at 19:51
  • 1
    \$\begingroup\$ This changes the behavior of the program as in the original the result is only printed after all values have been requested. \$\endgroup\$ Commented Jul 12, 2014 at 19:52
  • \$\begingroup\$ @JeroenVannevel thanks, I completely forgot that one. \$\endgroup\$ Commented Jul 12, 2014 at 19:53
3
\$\begingroup\$

As a general rule of thumb, you shouldn't nest ternary operators. It quickly (unintentionally) obfuscates the code. I would normally suggest writing separate functions for lowest and highest in this situation. You would then pass your numbers to those functions as an array. However, c# already has min and max methods for arrays.

After you've cleared the console:

int[] numbers = {num1,num2,num3,num4,num5};
int lowest = numbers.Min();
int highest = numbers.Max();

But arrays might not be the best solution here, as you have to hard code the number of elements in the array. Note that in your code, you ask the users for five numbers, but only use three of them to determine which number is highest and lowest. Bad juju. What you want is a list.

static void Main(string[] args)
{
    List<int> numbers = new List<int>();

    Console.WriteLine("Input Number");
    numbers.Add(int.Parse(Console.ReadLine()));

    Console.WriteLine("Input Number");
    number.Add(int.Parse(Console.ReadLine()));

    //etc.

    int[] output = numbers.ToArray();

    //above code to get min and max from our new array

    //display to user

Also note that I could have refactored out the console logic into its own method, so we don't have to repeat ourselves.

\$\endgroup\$
3
\$\begingroup\$

You really could use a loop there. From the looks of it, you're copy+pasting a bunch of lines of code whenever you want to change it to add more user inputs.

Start with a number that determines how many inputs you're taking:

var inputCount = 5;

If you want you could also prompt the user for that number.

Then you enter a loop where you ask the user for the numbers, and as @ckuhn203 mentioned you could have a List<int> to store the numbers:

var inputs = new List<int>();
while (inputs.Count < inputCount)
{
    var input = AskUserForAnyInteger(inputs.Count + 1);
    if (input.HasValue)
    {
        inputs.Add(input);
    }
}

Then if you want to prompt for 50 numbers instead of 5, you only have 1 thing to change!

Now this code is calling some AskUserForAnyInteger function, which could look like this:

private int? AskUserForAnyInteger(int n)
{
    Console.WriteLine("input {0}th number", n);
    int value;
    if (int.TryParse(Console.ReadLine(), out value))
    {
        return value;
    }
    else
    {
        Console.WriteLine("That's not a number!");
        return null;
    }
}

Notice the function validates the user's input - with your code, if the user enters abc your program will crash because int.Parse will throw an exception. Using int.TryParse instead allows you to try the parsing and avoid having to deal with an exception when the value can't be parsed into an int.

The int? notation is just shorthand for Nullable<int> - the function either returns an int, or null; then the calling code can check if there's a value by verifying the .HasValue property.


The lowest value in inputs can be retrieved with inputs.Min(), and inputs.Max() will give you the highest value.

This works because an int implements the IComparable<int> interface, which enables a List<int> to compare the values, and sort them.

\$\endgroup\$
2
  • \$\begingroup\$ I didn't know you could call Min and Max on a list. I must have missed that in the docs. Thanks. \$\endgroup\$
    – RubberDuck
    Commented Jul 12, 2014 at 15:52
  • 1
    \$\begingroup\$ You're right, Max and Min are Linq extension mehods :/ \$\endgroup\$ Commented Jul 12, 2014 at 15:56

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.