2
\$\begingroup\$

Background: I have created a program that allows a user to input information about their dog. The output is the bill they will receive commensurate their dog's weight.

The Problem: I believe I am doing this inefficiently. I don't like the way I have structured the current code and would like to improve upon my code. I would like critique from other programmers on how to improve this code

The Code:

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

namespace TheBarkingLotHasslerChasenB
{
    class Program
    {
        static bool isInt = false;
        static bool isDouble = false;

        static void Main(string[] args)
        {
            int id = 0;
            int age = 0;
            double weight = 0.0;

            id = HandleInput("Please enter your Owner ID", id);
            String name = HandleInput("Please enter the name of your dog");
            String breed = HandleInput("Please enter the breed of your dog");
            age = HandleInput("Please enter the age of your dog", age);
            weight = HandleInput("Please enter the weight of your dog", weight);

            DogOwner dogOwner = new DogOwner(id);

            Dog fido = new Dog(age, name, breed, weight);

            DetermineCost(fido.GetDogWeight());



            Console.Read();
        }

        private static int HandleInput(String input, int output)
        {
            do
            {
                Console.WriteLine(input);
                isInt = int.TryParse(Console.ReadLine(), out output);
                if (isInt)
                {
                    Console.WriteLine("");
                    return output;
                }

                Console.WriteLine("Please enter an integer value\n");


            } while (!isInt);

            return 0;

        }

        private static String HandleInput(String input)
        {
            String output;

            Console.WriteLine(input);

            output = Console.ReadLine();

            Console.WriteLine("");

            return output;
        }

        private static double HandleInput(String input, double output)
        {
            do
            {
                Console.WriteLine(input);
                isDouble = double.TryParse(Console.ReadLine(), out output);
                if (!isDouble)
                {
                    Console.WriteLine("Please enter a decimal value\n");
                }
                // If the user didn't enter a decimal, they are asked for input once again
                else
                {
                    Console.WriteLine("");
                    return output;
                }
            } while (!isDouble);

            return 0.0;
        }

        private static void DetermineCost(double weight)
        {
            if (weight < 15.0)
            {
                Console.WriteLine("Your bill for weekly day care is going to be: $55.00");
            }
            else if (weight > 15.0 && weight <= 30.0)
            {
                Console.WriteLine("Your bill for weekly day care is going to be: $75.00");
            }
            else if (weight > 30.0 && weight <= 80.0)
            {
                Console.WriteLine("Your bill for weekly day care is going to be: $105.00");
            }
            else
            {
                Console.WriteLine("Your bill for weekly day care is going to be: $125.00");
            }
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

1.- You should try to declare and instance variables at the same time. Instead of int age = 0; age = getUserInput(); this is better: int age = getUserInput();


2.- One method should do one thing, DetermineCost should 'determine cost', if you want to inform how much the cost is, then you should create another method (in this case I created PrintWeeklyCost())


3.- You should know where the variables are going to live, I mean, the scope of the variables. In you case isInt and isDouble should go only where they are going to be used.


4.- The if else if to see numeric ranges; it's really easy to make little mistakes. What if the dog is exactly 15 kg/pounds/whatever?. Will the cost be 125? An easy way to discover little errors is being consistent and, for me, that style helps a lot.


5.- What if the user doesn't enter text at all in HandleStringInput? (here I changed the name of the methods, I think that overloading methods can add more complexity to your program when it is not needed, now if you are learning go ahead!) Check for errors in input is a good practice !

class Program
{

    static void Main(string[] args)
    {
        int id = HandleIntegerInput("Please enter your Owner ID");
        String name = HandleStringInput("Please enter the name of your dog");
        String breed = HandleStringInput("Please enter the breed of your dog");
        int age = HandleIntInput("Please enter the age of your dog");
        double weight = HandleDoubleInput("Please enter the weight of your dog");

        DogOwner dogOwner = new DogOwner(id);
        Dog newDog = new Dog(age, name, breed, weight);

        double cost = DetermineCost(newDog.GetDogWeight());
        PrintWeeklyBill(cost);

        Console.Read();
    }


    private static int HandleIntInput(String input)
    {
        Console.WriteLine(input);

        int output;
        bool validInput = false;
        while(!validInput)
        {
            Console.WriteLine("Please enter an integer value\n");    
            validInput = int.TryParse(Console.ReadLine(), out output);
        }

        Console.WriteLine("");
        return output;
    }


    private static String HandleStringInput(String input)
    {
        Console.WriteLine(input);

        String output;
        bool validInput = false;
        while(String.IsNullOrEmpty(output))
        {
            Console.WriteLine("Please enter a valid text value\n");    
            output = Console.ReadLine();
        }

        Console.WriteLine("");
        return output;
    }


    private static double HandleDoubleInput(String input)
    {
        Console.WriteLine(input);

        double output;
        bool validInput = false;
        while(!validInput)
        {
            Console.WriteLine("Please enter an decimal value\n");    
            validInput = double.TryParse(Console.ReadLine(), out output);
        }

        Console.WriteLine("");
        return output;
    }


    private static double DetermineCost(double weight)
    {
        double cost;
        if (weight <= 15.0)
        {
            cost = 55.00;
        }
        else if (15.0 < weight && weight <= 30.0)
        {
            cost = 75.00;
        }
        else if (30.0 < weight && weight <= 80.0)
        {
            cost = 105.00;
        }
        else
        {
            cost = 125.00;
        }

        return cost;
    }

    private static void PrintWeeklyBill(double costPerWeek)
    {
        String costMsge = String.Format("Your bill for weekly day care is going to be: $", costPerWeek);
        Console.WriteLine(costMsge);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I understand the architecture of the methods such as HandleXInput, but I am receiving an error in my return block. Specifically, it says, "Use of unassigned local variable output". Is an acceptable fix assigning 0 to output? \$\endgroup\$ – Chasen Bettinger Feb 15 '17 at 19:20
  • \$\begingroup\$ Oh, my mistake! Yes, you can fix that in that way..! \$\endgroup\$ – mayo Feb 16 '17 at 2:36
3
\$\begingroup\$

Here are some more tips how to optimize it with C# 7.


Create a few general-purpose methods first. They'll take care of reading the input.

The main method is the ReadString. It'll keep trying to read until the input is not null or empty.

private static string ReadString(string prompt)
{
    var result = default(string);
    do Console.WriteLine(prompt); while (string.IsNullOrEmpty(result = Console.ReadLine()));
    return result;
}

The next method is the ReadInt32. Internally it uses ReadString but tries to parse the input as int as long as necessary. There's no need for an extra variable with C# 7.

private static int ReadInt32(string prompt)
{
    while (!int.TryParse(ReadString(prompt), out int result)) Console.WriteLine("Please enter an integer value.");
    return result;
}

The same does the ReadDecimal method. With weights and currencies you most probably want to use decimal and not double because of the possible rounding errors.

private static decimal ReadDecimal(string prompt)
{
    while (!decimal.TryParse(ReadString(prompt), out decimal result)) Console.WriteLine("Please enter an decimal value.");
    return result;
}

The old DetermineCost method can be splitted up into two methods.

One will only calculate the cost und utilizes the new switch with filters. To get the right value you just need to check the upper bound and start with the lowest one. You don't need to specify the range.

private static decimal CalcCost(decimal weight)
{
    switch (weight)
    {
        case decimal i when i <= 15: return 55.0m;
        case decimal i when i <= 30: return 75.0m;
        case decimal i when i <= 80: return 105.0m;
        default: return 125.0m;
    }
}

and the other one will print it. It uses string-interpolation with formatting.

private static void PrintCost(decimal cost)
{
    Console.WriteLine($"Your bill for weekly day care is going to be: ${cost:0.00}");
}

Lastly you assemble them into a working program:

static void Main(string[] args)
{
    var id = ReadInt32("Please enter your Owner ID");
    var name = ReadString("Please enter the name of your dog");
    var breed = ReadString("Please enter the breed of your dog");
    var age = ReadInt32("Please enter the age of your dog");
    var weight = ReadDecimal("Please enter the weight of your dog");

    var cost = CalcCost(weight);
    PrintCost(cost);
    Console.Read();
}
\$\endgroup\$
  • \$\begingroup\$ does the var datatype act as a generic? Can you put anything you want in that data type? Thank you for your submission, I learned a lot from your post \$\endgroup\$ – Chasen Bettinger Feb 15 '17 at 20:04
  • \$\begingroup\$ @ChasenBettinger kind of, it takes the type assigned to it but only once, if you write var a = 1; then a will be an int (forever) and so on. See var \$\endgroup\$ – t3chb0t Feb 15 '17 at 20:06
3
\$\begingroup\$
  • Don't return 0. Return void or don't use output and return the int.

    private static int HandleInput(String input, int output)
    
  • DetermineCost should return a string.

  • Separate calculation from reporting .
\$\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.