0
\$\begingroup\$

A friend and I at IT school tasked ourselves with writing something just for the hell of it. Trying to simulate in a very basic way something that you may want to do in the real world. We thought of the following problem that we could solve ourselves.

X, Y and X INC is in need of a program. They wish to promote the sales of their products in bigger quantity. They wish to give the following discounts to promote better sales.

  • If 1 to a 100 of a certain item is sold the company wishes to grant a 10 percent discount.
  • If the amount of the product sold is between 101 and 200 the company wishes to grant a 12 percent discount.
  • If the amount of the product sold is between 201 and 500 the company wishes to grant a 15 percent discount
  • If the amount of product sold is greater than 500 the discount should be 20 percent.

The program should take a price input and a quantity input and should give a total with the applicable discount to the end user without divulging the percentage of the discount.

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

namespace DiscountApp
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Please enter the quanitity of the product bought.");
            var x = Console.ReadLine();
            int x1 = int.Parse(x);

            Console.WriteLine("Please enter the price of the product bought.");
            var y = Console.ReadLine();
            double y1 = double.Parse(y);

            double discount = 0;
            double outValue = 0;

            if (x1 <= 100)
            {
                discount = 0.1;
                outValue = returnTotal(discount, y1, x1);
                Console.WriteLine("The total with the discount is R{0}",outValue);
            }
            else if (x1 > 100 && x1 <= 200)
            {
                discount = 0.12;
                outValue = returnTotal(discount, y1, x1);
                Console.WriteLine("The total with the discount is R{0}",outValue);
            }
            else if (x1 > 200 && x1 <= 500)
            {
                discount = 0.15;
                outValue = returnTotal(discount, y1, x1);
                Console.WriteLine("The total with the discount is R{0}",outValue);
            }
            else
            {   
                discount = 0.2;
                outValue = returnTotal(discount, y1, x1);
                Console.WriteLine("The total with the discount is R{0}",outValue);
            }


        }
        public static double returnTotal(double discount, double price, int quantity)
        {
            double Total = (price * quantity);
            double discountTotal = Total - (Total * discount);
            return discountTotal;
        }

    }

}

I'm not sure if a switch statement would be better. I'm not entirely sure how you would do multiple conditions in the switch statement.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Be aware that int.Parse/double.Parse will blow up in your face if the user enters something that isn't a number - you need to wrap those in a try catch statement to handle this possibility. \$\endgroup\$
    – Joe Clay
    Mar 28, 2017 at 9:14
  • 2
    \$\begingroup\$ In the real world these kind of things are usually data-driven, not hard-coded, so the sales department can change discount rules without having to ask a developer to update an application. \$\endgroup\$ Mar 28, 2017 at 9:57
  • \$\begingroup\$ Note how there's a lot of duplication in your code. The only difference between those if/else blocks is the discount. The outValue = ...; Console.WriteLine(...); part can easily be put after the if/else blocks. And a helper method that returns the discount percentage for a given quantity would also help to make the code more readable. Also, as Joe already mentioned, int.Parse and its cousins will throw exceptions on invalid input. Try using the TryParse variants instead. \$\endgroup\$ Mar 28, 2017 at 10:03
  • \$\begingroup\$ Any downvoters mind discussing their votes in a comment. \$\endgroup\$
    – Neil Meyer
    Mar 28, 2017 at 10:32
  • 1
    \$\begingroup\$ always use decimal for money instead of double. Double isn't a precise datatype. \$\endgroup\$ Mar 28, 2017 at 12:25

4 Answers 4

3
\$\begingroup\$

If you do the checks in ascending order, you don't need greater then checks. Since the only difference is the value of discount, you could use a dictionary.

static void Main(string[] args)
{
    Console.WriteLine("Please enter the quanitity of the product bought.");
    var x = Console.ReadLine();
    int x1 = int.Parse(x);

    Console.WriteLine("Please enter the price of the product bought.");
    var y = Console.ReadLine();
    double y1 = double.Parse(y);

    Dictionary<int, double> discountLevels = new Dictionary<int, double>
    {
        {100, 0.1},
        {200, 0.12},
        {500, 0.15}
    };

    double discount = 0.2;
    foreach (var discountLevel in discountLevels.OrderBy(discountLevel => discountLevel.Key))
    {
        if (x1 <= discountLevel.Key)
        {
            discount = discountLevel.Value;
            break;
        }
    }

    double outValue = 0;
    outValue = returnTotal(discount, y1, x1);
    Console.WriteLine("The total with the discount is R{0}", outValue);
}

The OrderBy ensures that the checks are done in the correct order.

\$\endgroup\$
3
  • \$\begingroup\$ I'd prefer the simper if for this example. But Dictionary is a possibility. \$\endgroup\$
    – SmallChess
    Mar 28, 2017 at 9:06
  • \$\begingroup\$ A dictionary is not the right data structure for this problem \$\endgroup\$ Mar 28, 2017 at 15:02
  • \$\begingroup\$ A list of KeyValuePair<int, double> or Tuple<int, double> could be used instead of the dictionary. In a real world app this check levels and discounts would probably be loaded from a datasource as list of specific objects. \$\endgroup\$
    – Martin
    Mar 29, 2017 at 6:53
2
\$\begingroup\$

You are clearly doing too many things in your main() method. You are reading user input, validating input and writing output.

These actions should be placed in well named methods.


The if..else if..else construct can be simplified for the first and second else if. Because you are checking in the if wether x1 <= 100 you don't need to check in the first else if again for x1 > 100.

Your else if conditions would then look like

if (x1 <= 100)
{
    discount = 0.1;
    outValue = returnTotal(discount, y1, x1);
    Console.WriteLine("The total with the discount is R{0}",outValue);
}
else if (x1 <= 200)
{
    discount = 0.12;
    outValue = returnTotal(discount, y1, x1);
    Console.WriteLine("The total with the discount is R{0}",outValue);
}
else if (x1 <= 500)
{
    discount = 0.15;
    outValue = returnTotal(discount, y1, x1);
    Console.WriteLine("The total with the discount is R{0}",outValue);
}
else
{   
    discount = 0.2;
    outValue = returnTotal(discount, y1, x1);
    Console.WriteLine("The total with the discount is R{0}",outValue);
}  

If you would only setting the discount inside this construct you can clean your code some more like so

        if (x1 <= 100)
        {
            discount = 0.1;
        }
        else if (x1 <= 200)
        {
            discount = 0.12;
        }
        else if (x1 <= 500)
        {
            discount = 0.15;
        }
        else
        {   
            discount = 0.2;
        }

        outValue = returnTotal(discount, y1, x1);
        Console.WriteLine("The total with the discount is R{0}",outValue);  

If we extract the "calculation" of the discount to a separate method like so

private static decimal CalculateDiscount(int quantity)
{
    if (quantity <= 100)
    {
        return 0.1m;
    }
    if (quantity <= 200)
    {
        return 0.12m;
    }
    if (quantity <= 500)
    {
        return 0.15m;
    }

    return 0.2m;
}  

the former code would look like so

decimal discount = CalculateDiscount(x1);
outValue = returnTotal(discount, y1, x1);
Console.WriteLine("The total with the discount is R{0}",outValue);  

Renaming and changing the return type of returnTotal to decimal CalculateTotal(decimal, decimal, int) should be done as well. Please note that I have changed the casing of the former Total field to use camelCase casing because thats whats expected based on the .Net Naming Guidelines.

private static decimal CalculateTotal(decimal discount, decimal price, int quantity)
{
    decimal total = (price * quantity);
    return total - (total * discount);
}

I have changed the scope of this method to private as well.


This all results in

static void Main(string[] args)
{
    Console.WriteLine("Please enter the quanitity of the product bought.");
    var x = Console.ReadLine();
    int x1 = int.Parse(x);

    Console.WriteLine("Please enter the price of the product bought.");
    var y = Console.ReadLine();
    var y1 = decimal.Parse(y);

    decimal discount = CalculateDiscount(x1);
    decimal outValue = CalculateTotal(discount, y1, x1);

    Console.WriteLine("The total with the discount is R{0}", outValue);  

}
private static decimal CalculateDiscount(int quantity)
{
    if (quantity <= 100)
    {
        return 0.1m;
    }
    if (quantity <= 200)
    {
        return 0.12m;
    }
    if (quantity <= 500)
    {
        return 0.15m;
    }

    return 0.2m;
}  
public static decimal CalculateTotal(decimal discount, decimal price, int quantity)
{
    decimal total = (price * quantity);
    return total - (total * discount);
}

There still is some for you to do, like

  • naming the variables x1 and y1 properly.
  • extracting the getting of the user input to a separate method
\$\endgroup\$
1
\$\begingroup\$

switch in C# and all programming languages I am aware of require a constant label. You should not use it to test for > and <=, that's the job for if.

https://msdn.microsoft.com/en-us/library/06tc147t.aspx

... Each case label specifies a constant value ...


returnTotal should be an anonymous function not a public method; the method name should also be PascalCase.


Otherwise, there's nothing wrong with your program, nor anything interesting. For your record, a "real" business application doesn't code the program logic in Main. What you did was like a HelloWorld learning exercise.

\$\endgroup\$
2
  • \$\begingroup\$ We made it up just to try and teach ourselves. Nowhere in this site's introductions does it say programs need to be interesting. \$\endgroup\$
    – Neil Meyer
    Mar 28, 2017 at 10:34
  • \$\begingroup\$ @NeilMeyer I didn't say it's off-topic. It's on-topic but uninteresting. \$\endgroup\$
    – SmallChess
    Mar 28, 2017 at 10:36
1
\$\begingroup\$

A few thoughts..

1.) Firstly "Main" is not where you should be doing the work - instead create a class to handle your discounting and have it expose a method that takes the quantity and price values as parameters and then that carries out both the determination of the discount level and returns the new discounted total.

2.) Probably beyond the scope of what you are asking here but as @Pieter Witvoet mentioned in a comment earlier the actual values for discount thresholds shouldn't be hardcoded into the application, these should be stored in a database or similar so that they can be changed without a developer having to edit code and re-compile the application. Another benefit of this will be that you can get the appropriate discount level from the database by querying which will get rid of all the messiness of having to use a chain of if statements. e.g.:

Simple discounts table for an MS-SQL Server database:

CREATE TABLE [dbo].[discount_levels](
    [quantity_from] [int] NULL,
    [quantity_to] [int] NULL,
    [discount] [decimal](18, 2) NULL
) ON [PRIMARY]

GO

discounts in table

which can be queried in SQL like this:

select * from discount_levels where quantity_from <= @quantity AND quantity_to >= @quantity

Now you can change discount quantity thresholds, percentages and even the number of discount levels without ever having to open Visual Studio! I won't go into how to connect and query the database here as it feels just a tad out of scope of this question and there are a wealth of resources on SE and elsewhere if you need help with that.

If for some arcane reason they absolutely have to be in the code then store them as consts in your discounter class instead, this will mean you only have one easy-to-find place to update them and the code is also much more readable as a result.

3.) When calculating money values decimals should be used instead of doubles, additionally you'll probably want to round the final value to 2dp before returning it to the user like so:

decimal finalvalue = Math.Round(someotherdecimal, 2, MidpointRounding.AwayFromZero)

4.) You're taking the quantity and price values as user input so you really should do some checking and error handling around that. Both Decimal and Int provide a TryParse method which will let you handle this very neatly:

decimal price;
bool parseResult = Decimal.TryParse(y, out price);
if (!parseResult)
{
    //report the error to the user and ask them to try again
}
else
{
   //all good, carry on!
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.