Parcel size classifier [closed]

Question

I submitted a technical assignment for a job interview process and I am stressing out wondering if my submission was any good. I would like to learn and be a better developer, so please give me pointers about what I could improve on.

GitHub link

using System;
namespace ParsetheParcel
{
    interface ICalculatable<Parcel>
    {
        string Calculator(Parcel p);
    }

    public class Parcel
    {
        //(fun fact, store currency as decimal and not float/double)
        public const decimal CostSmall = 5M;
        public const decimal CostMedium = 7.5M;
        public const decimal CostLarge = 8.5M;
        //Uppser limit placed on the weight
        public const double PackageWeight = 25;

        public double Weight { get; set; }
        public double Length { get; set; }
        public double Height { get; set; }
        public double Breadth { get; set; }

        public enum SizeSmall
        {
            length = 200,
            breadth = 300,
            height = 150,
        }
        public enum SizeMedium
        {
            length = 300,
            breadth = 400,
            height = 200,
        }
        public enum SizeLarge
        {
            length = 400,
            breadth = 600,
            height = 250
        }



    }

    class Program : ICalculatable<Parcel>
    {
        static void Main(string[] args)
        {
            //show welcome message
            Console.WriteLine("Kia ora, welcome to the trademe parcel calculator!\n");

            //start asking for inputs

            //ask for dimensions
            Console.WriteLine("Please enter the dimensions of your parcel and the unit (mm, cm or m) when promted:\n");
            Console.Write("Enter your dimensions for length: ");
            string length = GetInput();
            Console.Write("\nEnter your dimensions for breadth: ");
            string breadth = GetInput();
            Console.Write("\nEnter your dimensions for height: ");
            string height = GetInput();
            Console.Write("\nEnter the unit used for length, breadth and height (valid inputs: mm or cm or m): ");
            string dimenunit = GetUnitDimen();

            //ask for weight
            Console.WriteLine("\n Please Enter the weight of your parcel and the unit (g, kg) when promted: \n");
            Console.Write("Enter your weight: ");
            string weight = GetInput();
            Console.Write("\nEnter the unit used for used for weight (valid inputs: kg or g): ");
            string weightunit = GetUnitWeight();

            Parcel myparcel = Converter(length, breadth, height, dimenunit, weight, weightunit);

            Program p = new Program();


            Console.WriteLine("\n"+p.Calculator(myparcel));

            //make sure the console doesnt shutdown prematurely
            Console.WriteLine("\nPress Return key (enter) to Exit");
            Console.Read();
        }

        /* Function determines if what package type appropriate for the parcel*/
        public string Calculator(Parcel parcel)
        {
            //check if package meets minimum 25kg limit first
            if (parcel.Weight <= Parcel.PackageWeight)
            {
                //check if package is bigger than largest available first so it doesnt have to check through the other ifs
                if (parcel.Length > (double)Parcel.SizeLarge.length ||
                                        parcel.Breadth > (double)Parcel.SizeLarge.breadth || parcel.Height > (double)Parcel.SizeLarge.height)
                {
                    return "Sorry your Parcel is too large for us to ship!" +
                           "\nOur Largest Package: " + (double)Parcel.SizeLarge.length + "mm x" + (double)Parcel.SizeLarge.breadth + "mm x" + (double)Parcel.SizeLarge.height + "mm" +
                           "\nYour Dimensions:" + parcel.Length + "mm x" + parcel.Breadth + "mm x" + parcel.Height + "mm";
                }

                //check if package can fit in smallest first
                if (parcel.Length <= (double)Parcel.SizeSmall.length &&
                                        parcel.Breadth <= (double)Parcel.SizeSmall.breadth && parcel.Height <= (double)Parcel.SizeSmall.height)
                {
                    return "Your Parcel can fit in our small package type! The cost would be: " + Parcel.CostSmall.ToString("C");
                }
                else if (parcel.Length <= (double)Parcel.SizeMedium.length &&
                                        parcel.Breadth <= (double)Parcel.SizeMedium.breadth && parcel.Height <= (double)Parcel.SizeMedium.height)
                {
                    return "Your Parcel can fit in our Medium package type! The cost would be: " + Parcel.CostMedium.ToString("C");
                }
                else
                {
                    /*we already know the parcel is not greater than the largest availbe and we know that it wont fit in small or medium*/

                    return "Your Parcel can fit in our Large package type! The cost would be: " + Parcel.CostLarge.ToString("C");
                }
            }
            else //parcel too heavy
            {
                return "Sorry your Parcel is too heavy for us to ship! Our Limit: " + Parcel.PackageWeight + " Your Parcel Weight: " + parcel.Weight;
            }




        }

        /* Gets dimensions and weight values from the user*/
        private static string GetInput()
        {

            string Value; double result;
            do
            {
                Value = Console.ReadLine();
                if (!string.IsNullOrEmpty(Value) && double.TryParse(Value, out result))
                {
                    return Value;
                }
                else
                {
                    Console.WriteLine("Empty or incorrect input, please try again by entering a number only");
                }
            } while (string.IsNullOrEmpty(Value) || !double.TryParse(Value, out result));

            return Value;

        }

        /* Gets the unit input for the dimensions (mm, cm, m) */
        private static string GetUnitDimen()
        {

            string Value;
            do
            {
                Value = Console.ReadLine();
                if (!string.IsNullOrEmpty(Value) && Value.Equals("m", StringComparison.OrdinalIgnoreCase)
                                                        || Value.Equals("mm", StringComparison.OrdinalIgnoreCase)
                                                        || Value.Equals("cm", StringComparison.OrdinalIgnoreCase))
                {
                    return Value;
                }
                else
                {
                    Console.WriteLine("Empty or incorrect input, please try again");
                }
            } while (string.IsNullOrEmpty(Value) && !Value.Equals("m", StringComparison.OrdinalIgnoreCase)
                                                        || !Value.Equals("mm", StringComparison.OrdinalIgnoreCase)
                                                        || !Value.Equals("cm", StringComparison.OrdinalIgnoreCase));

            return Value;

        }

        /* Gets the unit input for the weight (kg, g) */
        private static string GetUnitWeight()
        {

            string Value;
            do
            {
                Value = Console.ReadLine();
                if (!string.IsNullOrEmpty(Value) && Value.Equals("kg", StringComparison.OrdinalIgnoreCase)
                                                        || Value.Equals("g", StringComparison.OrdinalIgnoreCase))
                {
                    return Value;
                }
                else
                {
                    Console.WriteLine("Empty or incorrect input, please try again");
                }
            } while (string.IsNullOrEmpty(Value) && !Value.Equals("kg", StringComparison.OrdinalIgnoreCase)
                                                        || !Value.Equals("g", StringComparison.OrdinalIgnoreCase));

            return Value;

        }

        /*convert dimensions to mm if it isnt already and convert weight to kg if it isnt already */
        private static Parcel Converter(string length, string breadth, string height, string dimenunit, string weight, string weightunit)
        {

            double result;
            double[] parcel = new double[4];

            if (double.TryParse(length, out result))//this should always pass as we have sanitized it in getInput Function
            {
                parcel[0] = result; //length
            }

            if (double.TryParse(breadth, out result))//this should always pass as we have sanitized it in getInput Function
            {
                parcel[1] = result; //breadth
            }

            if (double.TryParse(height, out result))//this should always pass as we have sanitized it in getInput Function
            {
                parcel[2] = result; //height
            }

            if (double.TryParse(weight, out result))//this should always pass as we have sanitized it in getInput Function
            {
                parcel[3] = result; //weight
            }

            //check units and convert to mm and kg if needed
            if (dimenunit.Equals("m", StringComparison.OrdinalIgnoreCase))
            {
                //convert m to mm for comparision
                parcel[0] = parcel[0] * 1000; //length convert to mm
                parcel[1] = parcel[1] * 1000; //breadth convert to mm
                parcel[2] = parcel[2] * 1000; //height convert to mm
            }
            else if (dimenunit.Equals("cm", StringComparison.OrdinalIgnoreCase))
            {
                //convert cm to mm for comparision
                parcel[0] = parcel[0] * 10; //length convert to mm
                parcel[1] = parcel[1] * 10; //breadth convert to mm
                parcel[2] = parcel[2] * 10; //height convert to mm
            }

            if (weightunit.Equals("g", StringComparison.OrdinalIgnoreCase))
            {
                //convert gram to kg for comprasion
                parcel[3] = parcel[3] / 1000;
            }

            Parcel p = new Parcel()
            {
                Length = parcel[0],
                Breadth = parcel[1],
                Height = parcel[2],
                Weight = parcel[3]
            };

            return p;
        }
    }

}

Answer 1

Usability

Your design is pretty string-heavy. If you want to build reusable functions, you should try to avoid this and work with more appropriate types given the context.

One such example is your interface design:

interface ICalculatable<Parcel>
{
    string Calculator(Parcel p);
}

What can consumers do with the returned string? This interface can only be used to render results to a UI like the console. Consider returning a decimal or a response class that stores all the required data for consumers to act upon.

return "Your Parcel can fit in our small package type! The cost would be: " + Parcel.CostSmall.ToString("C");

could be:

return Parsel.CostSmall;

Another example of returning the wrong type is GetInput. Internally you parse the data to double only to return the raw string. This seems a bit nuts to me.

   string Value; double result;
   // ..
   if (!string.IsNullOrEmpty(Value) && double.TryParse(Value, out result))
   {
        return Value;  // should return result instead
   }
   // ..

---

Design

ICalculatable<Parcel>

interface ICalculatable<Parcel>
{
    string Calculator(Parcel p);
}

• Start by defining proper names. The interface is used to calculate the cost price of a Parcel. The interface is hence a ICalculator, not ICalculatable. The parcel would be the latter.
• The name of the operation should be a verb specifying the action. CalculateCostPrice is a verb and is more specific than the generic Calculator.
• If using generics, use a type T both as generic class type and input parameter for the method. But since this interface is specific to calculating the cost price, I would not use a generic class, unless you define an interface for types that have a cost price. This would be an exercise left for you to explore.
• As already discussed, return a more useful return type. Let's assume all you want is the cost price, so a decimal would do.

/// <summary>
/// Performs calculations on Parcel instances.
/// </summary>
interface ICalculator
{ 
    /// <summary> Calculates the cost price of a Parcel in <unit>. </summary>
    decimal CalculateCostPrice(Parcel parcel);
}

General

• Consider making Parcel immutable. This ensures better encapsulation of its state.
• Enum names should be PascalCased, not camelCased.
• Don't let the entrypoint Program implement an interface. Create a specific ParcelCalculator class.
• Make sure to split API logic from end-user rendering. The string-based API is an anti-pattern for usability and reusability.
• I like the fact you put all user input parsing in Program, not in the API.

---

Conclusion

• I would focus on making a good design, think good about the arguments, return types and names of interfaces and methods. And don't forget to document interfaces to provide a clear Specification for consumers that focuses on Usability.
• Don't let rendering pollute the API. Remove all string-based messages from the API and put them in the presentation layer. This is called Separation of Concerns.

Answer 2

    public enum SizeSmall
    {
        length = 200,
        breadth = 300,
        height = 150,
    }
    public enum SizeMedium
    {
        length = 300,
        breadth = 400,
        height = 200,
    }
    public enum SizeLarge
    {
        length = 400,
        breadth = 600,
        height = 250
    }

Your use of the enum type is a little unusual. Normally an enum represents discrete values of the same kind, but yours each describes a parcel definition. A more "correct" way to define your dimension limits by enum types could be:

public enum Lengths 
{
  Small = 200,
  Medium = 300,
  Large = 400
}

public enum Breadths
{
  Small = 300,
  Medium = 400,
  Large = 200
}

public enum Heights 
{
  Small = 150,
  Medium = 200,
  Large = 250
}

---

You don't tell if the input dialog was given by the assignment, but anyway I find it a little laborious, not least because the user has to determine which side of the parcel is the length, the breadth and the height. Normally I would take the longest dimension for the length, but your lengths are actually shorter than the breadths?

Therefore I would help the user by letting him/her enter the dimensions more or less arbitrarily at one single prompt:

Start by letting the user enter units (it is counter intuitive that the units is entered after the values):

Console.Write("Enter units to use [m, cm, mm]: ");

Then the input with the max values as guidance:

Console.Write("Enter the size (L, B, H) (max: 400 x 200 x 250 mm) ");

Then when done, it's your programs job to first parse the input and then determine the right parcel by comparing the input with the parcel dimensions and maybe exchange the values for length and breadth if necessary etc.

---

Another thing that I would be frustrated about if I had to use your program, is that it evaluates the input as the last thing before it gives me the answer. A more user friendly approach would be to evaluate each input when it is entered - and letting the user enter a new value until a valid one is entered or until a an "exit program" token is specified.

---

You are very polite and descriptive in dialogue with the user, which is normally fine in human to human interaction (and maybe AI to human interaction) but in a plain calculation dialogue no one expects a detailed conversation so make it shorter and more precise. If needed you can provide a help function for each prompt or something like that.