10
\$\begingroup\$

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
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Are you familiar with method parameters and return values? \$\endgroup\$ – VisualMelon Aug 20 at 19:25
  • \$\begingroup\$ yeah, I tried that but unfortunately,I made it messier. \$\endgroup\$ – doctorWeird Aug 20 at 19:28
  • 3
    \$\begingroup\$ As you grow as a programmer, you may want to consider adding things like units of measure. "Enter weight of parcel:" would that be ounces, grams, or pounds? Also, why not double instead of integer. What if something is 1.5 ounces? \$\endgroup\$ – Rick Davin Aug 20 at 20:54
  • \$\begingroup\$ well yeah @Rick Davin, I actually thought about using double instead of integer but the criteria and scope of this sample code is to use integer :) \$\endgroup\$ – doctorWeird Aug 21 at 2:49
  • 1
    \$\begingroup\$ Somehow similar: codereview.stackexchange.com/q/104184/13424 \$\endgroup\$ – Adriano Repetti Aug 21 at 12:48
11
\$\begingroup\$

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
\$\endgroup\$
  • 2
    \$\begingroup\$ woahhh,dude I really appreciate this one. Thanks! \$\endgroup\$ – doctorWeird Aug 20 at 19:35
  • 2
    \$\begingroup\$ Beat me to it as usual, but you should use ReadKey(true) if you want to press any key to terminate ;) \$\endgroup\$ – VisualMelon Aug 20 at 19:42
  • \$\begingroup\$ @VisualMelon Well, if I have to chase you, I rarely catch you ;-) I'll take a break now :p \$\endgroup\$ – dfhwze Aug 20 at 19:53
  • 1
    \$\begingroup\$ @doctorWeird Next challenge for you would be to modify these methods to include a low and high limit for when you might modify your app to say "Enter a number between 1 and 10:" \$\endgroup\$ – Rick Davin Aug 20 at 20:51
  • 1
    \$\begingroup\$ @VisualMelon true to not display the pressed key - what a logic ;-] I would expect false to have this effect. \$\endgroup\$ – t3chb0t Aug 21 at 5:56
3
\$\begingroup\$

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.

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

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

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

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.