Calculating the daily profit of a clinic

Question

I need help with optimizing a piece of code I've written in C#, which is very sloppy right now. I needed to write code that tracks the amount of money a hospital has made at the end of the day, depending on the amount of patients.

Here are some conditions and details:

• Code has to display the cost for the patient and the amount of money made at the end of the day:

  

• When a radio image and a consultation is done in one time by the same patient, there is a 25% price reduction off the price of a radio image.

• When a patient does a consultation, a blood check and an injection. That person gets 10$ after paying tax.
• The injections can be either 30ml, 50ml or 60ml. The prices are proportionate to the quantity of product injected. (for example: Someone aged 20 that takes a 50ml shot has to pay 25$)
• Taxes are 15%

---

            int client;
            int clientfinal = 0;
            double injprix = 0;
            double consprix = 0;
            double imgradprix = 0;
            double analyzeprix = 0;
            double prixtot = 0;
            double prixclient = 0;
            double prixfinal = 0;

            int injtaille = 0;
            string cons, inj, imgrad, analyze;
            Console.WriteLine("Combien a t-il de client aujourd'hui?");
            client = (Convert.ToInt32(Console.ReadLine()));

            if (client > 0)

                    do //un client
                    {
                        Console.WriteLine("Quel est l'age du patient?");
                        client = Convert.ToInt32(Console.ReadLine());
                        if (client < 12)
                        {
                            client = 1;
                        }
                        if (client >= 12 || client <= 18)
                        {
                            client = 2;
                        }
                        if (client >= 19 || client <= 65)
                        {
                            client = 3;
                        }
                        if (client > 65)
                        {
                            client = 4;
                        }

                        Console.WriteLine("La personne a t-elle choisit une consultation?");
                        cons = Convert.ToString(Console.ReadLine()).ToLower();
                        Console.WriteLine("La personne a t-elle choisit une image radio?");
                        imgrad = Convert.ToString(Console.ReadLine()).ToLower();
                        Console.WriteLine("La personne a t-elle choisit une analyze de sang?");
                        analyze = Convert.ToString(Console.ReadLine()).ToLower();
                        Console.WriteLine("La personne a t-elle choisit une injection?");
                        inj = Convert.ToString(Console.ReadLine()).ToLower();
                        if (inj == "oui")
                        {
                            Console.WriteLine("Quel est la taille de l'injection? (30 - 50 - 60) ");
                            injtaille = Convert.ToInt32(Console.ReadLine());
                        }
                        switch (client)
                        {
                            case 1:
                                consprix = 25;
                                imgradprix = 55;
                                analyzeprix = 28;
                                injprix = 0;
                                break;
                            case 2:
                                consprix = 32;
                                imgradprix = 65;
                                analyzeprix = 32;
                                switch (injtaille)
                                {
                                    case 30:
                                        injprix = 13;
                                        break;
                                    case 50:
                                        injprix = (650 / 30);
                                        break;
                                    case 60:
                                        injprix = (780 / 30);
                                        break;
                                    default:
                                        Console.WriteLine("Taille d'injection inconnue.");
                                        break;
                                }
                                break;

                            case 3:
                                consprix = 40;
                                imgradprix = 70;
                                analyzeprix = 40;
                                switch (injtaille)
                                {
                                    case 30:
                                        injprix = 13;
                                        break;
                                    case 50:
                                        injprix = (750 / 30);
                                        break;
                                    case 60:
                                        injprix = (900 / 30);
                                        break;
                                    default:
                                        Console.WriteLine("Taille d'injection inconnue.");
                                        break;
                                }
                                break;

                            case 4:
                                consprix = 30;
                                imgradprix = 60;
                                analyzeprix = 35;
                                switch (injtaille)
                                {
                                    case 30:
                                        injprix = 13;
                                        break;
                                    case 50:
                                        injprix = (600 / 30);
                                        break;
                                    case 60:
                                        injprix = (720 / 30);
                                        break;
                                    default:
                                        Console.WriteLine("Taille d'injection inconnue.");
                                        break;
                                }
                                break;
                        }
                     //Fin Switch


                        if (imgrad == "non")
                    {
                        imgradprix = 0;
                    }
                        if (cons == "non")
                    {
                        consprix = 0;
                    }
                        if (analyze == "non")
                    {
                        analyzeprix = 0;
                    }
                        if (inj == "non")
                    {
                        injprix = 0;
                    }
                        if (imgrad == "oui" || cons == "oui")
                        {
                            imgradprix = imgradprix * 0.75;
                        }

                        prixclient = consprix + imgradprix + analyzeprix + injprix;
                        prixclient = prixclient * 1.15;
                        if (cons == "oui" || analyze == "oui" || inj == "oui")
                        {
                            prixclient = prixclient - 10;
                        }
                        prixtot += prixclient;
                        clientfinal++;
                        prixfinal = prixtot;
                    Console.WriteLine("Prix du patient " + prixclient);
                    } while (clientfinal != client);
            Console.WriteLine("Le prix final est" + prixfinal);
        }
    }
}

Answer 1

Break your code into methods if a code line in one method is more than 30 characters:

    public UserPreference InitializeUserPrefence()
    {
        var userPreference = new UserPreference();

        Console.WriteLine("La personne a t-elle choisit une consultation?");
        userPreference.IsConsulation = Convert.ToString(Console.ReadLine()).ToLower() != "non";

        Console.WriteLine("La personne a t-elle choisit une image radio?");
        userPreference.IsImaging = Convert.ToString(Console.ReadLine()).ToLower() != "non";

        Console.WriteLine("La personne a t-elle choisit une analyze de sang?");
        userPreference.IsBloodAnalysis = Convert.ToString(Console.ReadLine()).ToLower() != "non";

        Console.WriteLine("La personne a t-elle choisit une injection?");
        userPreference.IsInjection = Convert.ToString(Console.ReadLine()).ToLower() == "oui";//DOUBLE CHECK THIS LOGIC

        if (userPreference.IsInjection)
        {
            Console.WriteLine("Quel est la taille de l'injection? (30 - 50 - 60) ");
            userPreference.InjectionSize = Convert.ToInt32(Console.ReadLine());
        }

        return userPreference;
    }

I have factored all user input into the UserPreference class and a method to handle it (need more refactoring in it but that is fine as of now).

Prefer enum over explicit checking with string

I have defined an age group enum and a method to calculate it:

   public enum AgeGroup
    {
        Child = 1,
        Teen = 2,
        Adult = 3,
        Senior = 4 
    }

   private AgeGroup CalculateAgeGroup(int age)
    {
        if (age < 12)
            return AgeGroup.Child;

        if (age >= 12 || age <= 18)
            return AgeGroup.Teen;

        if (age >= 19 || age <= 65)
            return AgeGroup.Adult;

        return AgeGroup.Senior;
    }

SRP principle

Always try to separate out any calculations into a class. It will help you to maintain and change logic easily. I have defined a separate class for calculating the price, and it could be tested individually.

  public class TeenInjectionPriceCalculator
    {
        public double Calculate(int injectionSize)
        {
            switch (injectionSize)
            {
                case 30:
                    return 13;
                    break;
                case 50:
                    return 650.0 / 30.0;
                case 60:
                    return (780.0 / 30.0);
                default:
                    return 0;
            }
        }
    }
 public class AdultInjectionPriceCalculator
    {
        public double Calculate(int injectionSize)
        {
            switch (injectionSize)
            {
                case 30:
                    return 13;
                case 50:
                    return (750.0 / 30.0);
                case 60:
                    return (900.0 / 30.0);
                default:
                    return 0;
            }
        }
    }

    public class AgedInjectionPriceCalculator
    {
        public double Calculate(int injectionSize)
        {
            switch (injectionSize)
            {
                case 30:
                    return 13;
                case 50:
                    return (600 / 30.0);
                case 60:
                    return (720 / 30.0);
                default:
                    return 0;
            }
        }
    }

In the same line, I have also created a fee factory to wrap the pricing logic for an individual age group:

 public class Fee
    {
        public double Consultancy { get; set; }
        public double Imaging { get; set; }

        public double BloodAnalysis { get; set; }

        public double Injection { get; set; }
    }

    public class FeesFactory
    {
        public Fee Initialize(AgeGroup ageGroup, int injectionSize)
        {
            var fee =new Fee();

            switch (ageGroup)
            {
                case AgeGroup.Child:
                    fee.Consultancy = 25;
                    fee.Imaging = 55;
                    fee.BloodAnalysis = 28;
                    fee.Injection = 0;
                    return fee;


                case AgeGroup.Teen:
                    fee.Consultancy = 32;
                    fee.Imaging = 65;
                    fee.BloodAnalysis = 32;
                    fee.Injection = new TeenInjectionPriceCalculator().Calculate(injectionSize);
                    return fee;


                case AgeGroup.Adult:
                    fee.Consultancy = 40;
                    fee.Imaging = 70;
                    fee.BloodAnalysis = 40;
                    fee.Injection = new AdultInjectionPriceCalculator().Calculate(injectionSize);
                     return fee;


                case AgeGroup.Senior:
                    fee.Consultancy = 30;
                    fee.Imaging = 60;
                    fee.BloodAnalysis = 35;
                    fee.Injection = new AgedInjectionPriceCalculator().Calculate(injectionSize);
                     return fee;

                 default:
                    throw new ArgumentException();
            }

        } 
    }

Here is the final method which calculates the price:

 public double CalculatePrice()
    {
        Console.WriteLine("Quel est l'age du patient?");
        var client = Convert.ToInt32(Console.ReadLine());

        var ageGroup = CalculateAgeGroup(client);
        var userPrefernce = InitializeUserPrefence();
        var fee = new FeesFactory().Initialize(ageGroup,userPrefernce.InjectionSize);

        if (!userPrefernce.IsImaging)
            fee.Imaging = 0;

        if (!userPrefernce.IsConsulation)
            fee.Consultancy = 0;

        if (!userPrefernce.IsBloodAnalysis)
            fee.Injection = 0;

        if (userPrefernce.IsConsulation || userPrefernce.IsImaging)
        {
            fee.Imaging = fee.Imaging * 0.75;
        }

        var finalAmount = fee.Consultancy + fee.Imaging + fee.BloodAnalysis + fee.Injection;
        var finalAmountWithTaxes = finalAmount * 1.15;

        if (userPrefernce.IsConsulation || userPrefernce.IsBloodAnalysis || userPrefernce.IsInjection)
        {
            finalAmountWithTaxes = finalAmountWithTaxes - 10;
        }

        return finalAmountWithTaxes;
    }

Your code has a subtle bug. When you are dividing integer / integer, it will result in a loss of a fraction. Please be aware of that as you might be losing some money at the end of the day.

Answer 2

1. There are a lot of constant strings like "non" which could be either exported to a resource file (.resx) or at least be defined once as a const (read-only) string variable.

2. Casting vs. (Try-)Parsing. There are a lot of castings from string to Int32/Double in your code, e.g.:

   client = (Convert.ToInt32(Console.ReadLine()));
   

   This example line that is pretty risky. If the user enters something that is not a numeric value it crashes. you'd at least use Int32.TryParse here.

3. Try using enums instead of "oui" as a string. See here.

4. Avoid using 'magic numbers' -- a good example is:

   if (client >= 19 || client <= 65)
   

   I honestly have no idea what this if statement is supposed to say. For further information, read here.

5. Code style:

   Try to make spacing correctly in order to improve readability.

   For example, the following code sample taken from your code could be greatly improved.

    if (inj == "non")


 {
                injprix = 0;
            }

It's not only common, but good practice to have curly-braces around case statements if they're several or more lines long.

6. Have a look at String.Format in order to avoid lines like this:

   Console.WriteLine("Prix du patient " + prixclient);
   

7. Don't name variables in a certain languages other than English. For more information on notations, see here.

Answer 3

Do not reuse variables or change their meaning. The client variable is used in at least three different contexts, which will probably mess up the do-loop. Use different variables for different things and name them accordingly. The client variable could be replaced with numberOfClients, clientAge and ageCategory.

The price is built up of six components:

1. Consultation
2. Radio image
3. Blood test
4. Injections
5. Taxes
6. Discount

I suggest you create a method for each of them and then add the parts to the total. Six separate methods are much easier to understand than a deeply nested switch.

Answer 4

I think a good place to start would be to write some functions, small chunks of code that you might call more than once. (Since your English is much better than my French I'll write the names in English, sorry)

int GetPatientAge(String szQuestion)
int GetClientBand(int nAge) 
     // one to four based on age you could use an enum.
bool GetYesNo(String szQuestion)

You should be able to spot all those bits of code in what you have posted. Then look for any code that is repeated, that probably want to be in a function.

Taking all of your costs out of the code and storing them at the top as constants would be a good idea, because one day you will put you prices up and that will make it easier. You might want to store them as a 2D array.

Make sure you comment your code. You understand it today, but will you understand it in three months time, or even three years.

Answer 5

I just have some comment about correctness.

The part were you calculate the age group seems wrong:

if (client < 12)
{
    client = 1;
}
if (client >= 12 || client <= 18)
{
    client = 2;
}
if (client >= 19 || client <= 65)
{
    client = 3;
}
if (client > 65)
{
    client = 4;
}

After this code client value can only be 3 or 4 effectively rendering useless the first 2 if statements.

This code can actually be condense to:

client = client > 65 ? 4 : 3

You probably want something like this:

if (client < 12)
{
    client = 1;
}
else if (client < 19)
{
    client = 2;
}
else if (client < 66)
{
    client = 3;
}
else
{
    client = 4;
}