Repetitive validation of Console inputs

Question

I have this code that asks the user for inputs and validates the inputs if they are Integers (no other inputs are allowed). I'd like to ask for some help in terms of elegance of the code or performance, or just the overall code.

Also, I have found myself repeating these lines of code

Console.Write("Enter weight of parcel:  ");
string input = Console.ReadLine();

and I think I really need to refactor the code because of this, but can't think of a simple and straightforward way to refactor it.

Here is my code below:

class Program
{
    private static int _depth = 0;
    private static int _height = 0;
    private static int _weight = 0;
    private static int _width = 0;
    private static bool _isInvalidInput = true;
    static void Main(string[] args)
    {
        GetWeightInput();
        GetHeightInput();
        GetWidthInput();
        GetDepthInput();

        Console.ReadLine();
    }
    private static void GetWeightInput()
    {
        while(_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _weight))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }


        _isInvalidInput = true;//reset value to true
    }
    private static void GetHeightInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _height))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }


        _isInvalidInput = true;//reset value to true
    }
    private static void GetWidthInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _width))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }


        _isInvalidInput = true;//reset value to true
    }
    private static void GetDepthInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _width))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }


        _isInvalidInput = true;//reset value to true
    }
}

Answer 1

As you suggested, you have a lot of redundant code. And rightfully so you want to adhere to the DRY principle.

All these input methods have the same pattern internally..

static void Main(string[] args)
{
    GetWeightInput();
    GetHeightInput();
    GetWidthInput();
    GetDepthInput();

    Console.ReadLine();
}

Road to DRY

..ideally you would like to be able to call them like this:

static void Main(string[] args)
{
    var weight = AskInteger("Enter weight of parcel: ");
    var height = AskInteger("Enter height of parcel: ");
    var width = AskInteger("Enter width of parcel: ");
    var depth = AskInteger("Enter depth of parcel: ");

    Console.WriteLine("press any key to terminate the application..");
    Console.ReadKey(true);
}

Advanced road to DRY

Or if you would provide a complex object Request with 4 properties and a lambda to provide both a message to the user as a setter expression to materialise the request:

static void Main(string[] args)
{
    var request = new Request();
    AskInteger(request, x => x.Weight);
    AskInteger(request, x => x.Height);
    AskInteger(request, x => x.Width);
    AskInteger(request, x => x.Depth);

    Console.WriteLine("press any key to terminate the application..");
    Console.ReadKey(true);
}

How you would implement AskInteger is a challenge I leave to you.

---

Misc

• Don't pollute your code with regions that have no added value. #region User Weight Input Validation
• Try to avoid meaningless comments _isInvalidInput = true;//reset value to true

Answer 2

In GetWidthInput the prompt is: "enter weight of parcel:". This is clearly wrong.

To see whether it's worth merging all the code into a single function you should first fill in the missing TODOs for validating the numbers to see whether the code structure stays the same. It probably does. After that, the custom validation code can be added via an anonymous function as well, as explained in the other answer.

Answer 3

To me, since a parcel is an object, you should make it an Object:

class Parcel
{
    int Weight { get; set; }
    int Height { get; set; }
    int Width { get; set; }
    int Depth { get; set; }
    public Parcel() { }


}

One way to gather the inputs and fill the object with them, is to use a Builder.

To cut down on a lot of the repetition, you can store the prompts in a string array:

private static readonly string[] DimensionsPrompts = new string[]
{
    "Enter weight of parcel: ",
    "Enter height of parcel: ",
    "Enter width of parcel: ",
    "Enter depth of parcel: "
};

To facilitate the builder you'll also need a constructor that takes an int[].

The builder could look like this:

public static Parcel BuildParcel(string[] prompts, TextWriter sOut, TextReader sIn)
{
    if (prompts.Length < 4)
    {
        throw new ArgumentException("`prompts` must have 4 elements");
    }
    int[] values = new int[4];
    for(int i = 0; i < 4; ++i)
    {
        sOut.Write(prompts[i]);
        bool correct = false;
        while (!correct)
        {
            correct = int.TryParse(sIn.ReadLine(), out values[i]);
            if (!correct)
            {
                sOut.WriteLine("Input must an integer,  Try again");
            }
        }
    }
    return new Parcel(values);
}

The whole thing could look like this:

class Parcel
{
    int Weight { get; set; }
    int Height { get; set; }
    int Width { get; set; }
    int Depth { get; set; }
    public Parcel() { }
    public Parcel(params int[] values)
    {
        if(values.Length < 4)
        {
            throw new ArgumentException("`values` must have 4 elements");
        }
        Weight = values[0];
        Height = values[1];
        Width = values[2];
        Depth = values[3];
    }

    public static Parcel BuildParcel(string[] prompts, TextWriter sOut, TextReader sIn)
    {
        if (prompts.Length < 4)
        {
            throw new ArgumentException("`values` must have 4 elements");
        }
        int[] values = new int[4];
        for(int i = 0; i < 4; ++i)
        {
            sOut.Write(prompts[i]);
            bool correct = false;
            while (!correct)
            {
                correct = int.TryParse(sIn.ReadLine(), out values[i]);
                if (!correct)
                {
                    sOut.WriteLine("Input must an integer,  Try again");
                }
            }
        }
        return new Parcel(values);
    }
}

private static readonly string[] DimensionsPrompts = new string[]
{
    "Enter weight of parcel: ",
    "Enter height of parcel: ",
    "Enter width of parcel: ",
    "Enter depth of parcel: "
};

One advantage to using a class like this, you can easily add a method that will calculate the base cost, based on its properties

Answer 4

There are already good reviews, but there are some other points that should be addressed.

You don't need to use global static variables in your case. Your methods should only use local variables because you always reset your variables in your methods, which makes the global variables useless. A method named Get... should always return something, it just makes sense.

Second, it's a bad practice to have boolean variables that represent negative conditions. I mean that it's confusing to have isInvalidInput = false instead of isValidInput = true. It's most of the time clearer to use positive variables. If you used the break keyword, you could skip the use of the isInvalidInput and remove the else in your code. You can also (assuming you're using C#7) use out int width instead of declaring int width then use the out variable. Lastly, the word integer means something to developers, but not that much to non-IT people. If your program is used by non IT people, you should consider rephrasing it.

Final version :

private static int GetDepthInput()
{
    while (true)
    {
        Console.Write("Enter weight of parcel:  ");
        string input = Console.ReadLine();

        if (int.TryParse(input, out int width))
        {
            break;
        }

        Console.WriteLine("***The input is invalid. (Decimal numbers aren't allowed)***");
    }

    return input;
}

I didn't address the other points that are written in the other 2 reviews but I also think they should be implemented.