22
\$\begingroup\$

There were a few recent questions which had a challenge like this:

The subway shop provides catering for meetings and other events. All sandwich platters are $40 each. Cookie platters are $20 each. Beverages are not included in catering service. Write a program that prompts the user for the number of sandwich platters and the number of cookie platters. The program should compute the total cost of the order (including a 6% tax).

Input statements you should code
- How many sandwich platters?
- How many cookie platters?

Output statements your program should display:
- Price of sandwich platter(s):
- Price of cookie platter(s):
- Price before taxes:
- Tax:
- Price plus taxes:

Code the program using Scanner Console.ReadLine().

This was originally intended for Java, evidently, but I made an implementation in C#.

This is the first time I write anything more substantive than 'Hello, World!' in C#, so all recommendations are welcome!

Working demo on DotNetFiddle.net

using System;
using System.Collections.Generic;
using System.Text;

public class BuzzwaySubs
{
    private static Dictionary<string, decimal> cateringMenu = new Dictionary<string, decimal>()
    {
        {"Sandwich Platter", 39.99M},
        {"Cookie Platter", 19.99M}
    };
    public const decimal SALES_TAX = 0.06M;

    public static void processCustomer()
    {
        Console.WriteLine("Welcome to Buzzway Subs!");
        Console.WriteLine("May I take your order?");
        var order = takeCateringOrder();
        var subTotal = calculateSubTotal(order);
        var salesTax = calculateSalesTax(order);
        var total = calculateTotal(order);
        Console.WriteLine
        (
            "Subtotal: {0:0.00}\n"+
            "Tax: {1:0.00}\n" +
            "Total: {2:0.00}\n",
            subTotal, 
            salesTax, 
            total
        );
        decimal payment = getPaymentFromCustomer(total);
        processPayment(total, payment);
        printReceipt(order, payment);
        Console.WriteLine("Thank you for shopping at Buzzway!");

    }

    public static void printCateringMenu()
    {
        Console.WriteLine("Catering Menu:");
        foreach (var product in cateringMenu)
        {
            Console.WriteLine("{0}: ${1:0.00}", product.Key, product.Value);
        }
        Console.WriteLine();
    }

    public static Dictionary<string, int> takeCateringOrder()
    {
        printCateringMenu();

        Dictionary<string, int> order = new Dictionary<string, int>();      
        foreach (var product in cateringMenu)
        {
            string input;
            int quantity;
            bool isValidQuantity;
            bool proceedToNextItem = false;

            while (!proceedToNextItem)
            {   
                Console.WriteLine("Purchase how many of {0} for ${1:0.00} each?", product.Key, product.Value);
                input = Console.ReadLine();
                isValidQuantity = int.TryParse(input, out quantity);
                if (!isValidQuantity)
                {
                    Console.WriteLine("Invalid number input.");
                }
                else if (isValidQuantity && quantity == 0)
                {
                    //don't add item
                    proceedToNextItem = true;
                }
                else
                {
                    order.Add(product.Key, quantity);
                    proceedToNextItem = true;
                }
            }
        }
        return order;
    }

    public static decimal calculateSubTotal(Dictionary<string, int> order)
    {
        decimal subTotal = 0M;
        int itemQty = 0;
        foreach (var item in order)
        {
            itemQty = item.Value;
            decimal costOfItems = itemQty * cateringMenu[item.Key];
            subTotal += costOfItems;
        }
        return subTotal;
    }

    public static decimal calculateSalesTax(Dictionary<string, int> order)
    {
        return calculateSubTotal(order) * SALES_TAX;
    }

    public static decimal calculateTotal(Dictionary<string, int> order)
    {
        decimal total = calculateSubTotal(order) + calculateSalesTax(order);
        return total;
    }

    public static decimal getPaymentFromCustomer(decimal total) 
    {
        decimal payment = 0M;
        bool isValidPayment = false;
        while (!isValidPayment)
        {
            Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);
            var input = Console.ReadLine();
            isValidPayment = decimal.TryParse(input, out payment);
            if (!isValidPayment) 
            {
                Console.WriteLine("Invalid payment amount. Please try again.");
            }
            else
            {
                isValidPayment = true;
            }
        }
        return payment;
    }

    public static decimal processPayment(decimal total, decimal payment)
    {
        bool isPaymentEnough = false;
        decimal change = 0;
        while (!isPaymentEnough) 
        {
            if (payment == total)
            {
                isPaymentEnough = true;
            }
            else if (payment < total) 
            {
                Console.WriteLine
                (
                    "Payment ${0:0.00} is not enough for total ${1:0.00}"
                    ,payment, 
                    total
                );
                payment = getPaymentFromCustomer(total);
            }
            else
            {
                isPaymentEnough = true;
                change = payment + -total;
                Console.WriteLine("Your change is ${0:0.00}.", change);
            }
        }
        return change;
    }

    public static void printReceipt(Dictionary<string, int> order, decimal payment)
    {
        StringBuilder receipt = new StringBuilder("");
        Console.WriteLine("--- Receipt ---");
        foreach (var item in order)
        {
            Console.WriteLine
            (
                "{0} {1} ${2} ea. ${3}",
                item.Value,
                item.Key,
                cateringMenu[item.Key],
                item.Value * cateringMenu[item.Key]
            );
        }
        var subTotal = calculateSubTotal(order);
        var salesTax = calculateSalesTax(order);
        var total = calculateTotal(order);
        Console.WriteLine("Subtotal: ${0:0.00}", subTotal);
        Console.WriteLine("Tax: ${0:0.00}", salesTax);
        Console.WriteLine("Total: ${0:0.00}", total);
        Console.WriteLine("Payment: ${0:0.00}", payment);
        decimal change = processPayment(total, payment);
    }
}

public class Program
{
    public static void Main()
    {
        new BuzzwaySubs();
        BuzzwaySubs.processCustomer();
    }
}

Example run:

Welcome to Buzzway Subs!
May I take your order?
Catering Menu:
Sandwich Platter: $39.99
Cookie Platter: $19.99

Purchase how many of Sandwich Platter for $39.99 each?
1
Purchase how many of Cookie Platter for $19.99 each?
2
Subtotal: 79.97
Tax: 4.80
Total: 84.77

Your total due is $84.77. 
Pay how much?
85
Your change is $0.23.
--- Receipt ---
1 Sandwich Platter $39.99 ea. $39.99
2 Cookie Platter $19.99 ea. $39.98
Subtotal: $79.97
Tax: $4.80
Total: $84.77
Payment: $85.00
Your change is $0.23.
Thank you for shopping at Buzzway!
\$\endgroup\$
1
  • \$\begingroup\$ Your items seem to be one cent too cheap compared to the problem statement \$\endgroup\$ – Hagen von Eitzen Dec 6 '15 at 14:55
17
\$\begingroup\$

First, keep track of your instances of a class:

new BuzzwaySubs();
BuzzwaySubs.processCustomer();

That should be:

BuzzwaySubs restaurant = new BuzzwaySubs();  // or `var restaurant`
restaurant.processCustomer();

As it is, the first line is utterly useless. Additionally, this only works because your methods are all static.

Keeping track of your instances is important because what happens when you have two restaurants? You need to know which restaurant is controlled by which class instance so you can manage them appropriately.


Second, declare your variables in the tightest scope possible:

int itemQty = 0;
foreach (var item in order)
{
    itemQty = item.Value;
    decimal costOfItems = itemQty * cateringMenu[item.Key];
    subTotal += costOfItems;
}

That variable should be declared in the foreach loop. You have this probably in a great many places.

foreach (var item in order)
{
    int itemQty = item.Value;
    decimal costOfItems = itemQty * cateringMenu[item.Key];
    subTotal += costOfItems;
}

This is important for many reasons, including keeping your variables from leaking information to other sections of the program, releasing memory when you aren't using it, and more.


Third, your naming does not follow standard C# naming practices:

public static void printCateringMenu()

Public methods are named with PascalCase.

public const decimal SALES_TAX = 0.06M;

Public fields are also named with PascalCase.


Fourth, you've got a bad case of the static fever. Please, oh please, why is every method in BuzzwaySubs static? You might as well have made the class static! What if you have different BuzzwaySubs restaurants? Static instances are only created once because they belong to the type, not the instance, so you are stuck with only many restaurants, but only one menu, one checkout system, and one of everything else that should be restaurant specific?

Also, if you had not made your methods static, that crazy logic in Main() would have been illegal (is that, in fact, what lead to this?).


Fifth, R# is warning me about some unnecessary logic:

if (!isValidQuantity)
{
    Console.WriteLine("Invalid number input.");
}
else if (isValidQuantity && quantity == 0)
{
    //don't add item
    proceedToNextItem = true;
}

The isValidQuantity check in the else if is unneeded because you check it in the if above. If it is false, your logic will never get past the first if.

StringBuilder receipt = new StringBuilder("");
decimal change = processPayment(total, payment);

Unused variables.

else
{
    isValidPayment = true;
}

Unnecessary assignment - value is already true.

These are all dead code for the programmer to read that the compiler is likely (and hopefully) optimizing away.


Sixth, keep your variable declaration consistent:

var total = calculateTotal(order);
decimal change = processPayment(total, payment);

Consistency is next to cleanliness.


Seventh, please use appropriate access modifiers:

public const decimal SALES_TAX = 0.06M;

When are you ever going to have to access that from outside the method? That should be private. Some of your methods should probably be private as well. The access of fields, methods, etc. is very important to control to prevent outsiders from seeing what you are doing. When I create an instance of this class, which should I be able to see your background information and what your methods are all doing?


Eighth, if you need to run a loop at least once, consider using a do-while loop:

bool isValidPayment = false;
while (!isValidPayment)
{
    Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);
    string input = Console.ReadLine();
    isValidPayment = decimal.TryParse(input, out payment);
    if (!isValidPayment)
    {
        Console.WriteLine("Invalid payment amount. Please try again.");
    }
}

Becomes:

bool isValidPayment;
do
{
    Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);
    string input = Console.ReadLine();
    isValidPayment = decimal.TryParse(input, out payment);
    if (!isValidPayment)
    {
        Console.WriteLine("Invalid payment amount. Please try again.");
    }
} while (!isValidPayment);

This is not required, but it shows more clearly that the loop must execute at least once by definition, and it prevents someone from breaking your program by changing the bool isValidPayment = false; to bool isValidPayment = true;.


Ninth, you can make this dictionary readonly:

private static Dictionary<string, decimal> _cateringMenu = new Dictionary<string, decimal>()
{
    {"Sandwich Platter", 39.99M},
    {"Cookie Platter", 19.99M}
};
private static readonly Dictionary<string, decimal> _cateringMenu = new Dictionary<string, decimal>()
{
    {"Sandwich Platter", 39.99M},
    {"Cookie Platter", 19.99M}
};

This will prevent you from overwriting it with a new dictionary.


Ten, you can use continue and break in loops. break will exit the loop, while continue will evaluate the condition and start at the beginning of the loop. Using these statements can help you remove some of your flags. For example, instead of using proceedToNextItem = true; to exit the loop when the condition is re-evaluated as the last statement in a successful loop, you can use break.


Eleven, you have a bug in your logic. You do not check if the customer orders a negative amount of food:

isValidQuantity = int.TryParse(input, out quantity);
if (!isValidQuantity)
{
    Console.WriteLine("Invalid number input.");
}
else if (isValidQuantity && quantity == 0)
{
    //don't add item
    proceedToNextItem = true;
}
else
{
    order.Add(product.Key, quantity);
    proceedToNextItem = true;
}

You can change the first condition to: if (!isValidQuantity || quantity < 0) to fix this.


Twelve, you can use string interpolation, if you are using C# 6 (All credit to Jeroen Vannevel. This is signaled by placing a $ in front of the string, such as:

Console.WriteLine($"Your total due is ${total:0.00}. \nPay how much?");

Instead of:

Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);

Thirteen, also credit to Jeroen Vannevel, you can use expression-bodied members when your methods are only one expression long:

public decimal CalculateSalesTax(Dictionary<string, int> order)
{
    return CalculateSubTotal(order) * SalesTax;
}

Becomes:

public decimal CalculateSalesTax(Dictionary<string, int> order) =>
    CalculateSubTotal(order) * SalesTax;
\$\endgroup\$
0
11
\$\begingroup\$

Use Linq when practical

C# is a multi-paradigm language, so you are free to use a functional style when it improves the code: calculateSubTotal can be written using Linq as a simple combination of Select and Sum, making it a one(or two)-liner while also making the intent clearer.

Here is a practical example:

return order.Select(item => item.Value * cateringMenu[item.Key]).Sum();

Use less boolean flags

Your code is full of boolean flags. I suggest using while(true) and using the break keyword to get out of loops as a simplification.

Do not waste lines

Of course writing the shortest code possible is not good. But writing what could be very concisely expressed in too many lines obscures the true logic of the code, and the code feels boiler-plat-y.

I shortened getPaymentFromCustomer from:

public static decimal getPaymentFromCustomer(decimal total) 
{
    decimal payment = 0M;
    bool isValidPayment = false;
    while (!isValidPayment)
    {
        Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);
        var input = Console.ReadLine();
        isValidPayment = decimal.TryParse(input, out payment);
        if (!isValidPayment) 
        {
            Console.WriteLine("Invalid payment amount. Please try again.");
        }
        else
        {
            isValidPayment = true;
        }
    }
    return payment;
}

to:

public static decimal getPaymentFromCustomer(decimal total) 
{
    decimal payment = 0M;
    while (true)
    {
        Console.WriteLine("Your total due is ${0:0.00}. \nPay how much?", total);
        if (!decimal.TryParse(Console.ReadLine(), out payment)) 
        {
            Console.WriteLine("Invalid payment amount. Please try again.");
        }
        else
        {
             return payment;
        }
    }
}

Functionality is identical, but my version shows the logic more directly.

Another example is processPayment:

From:

public static decimal processPayment(decimal total, decimal payment)
{
    bool isPaymentEnough = false;
    decimal change = 0;
    while (!isPaymentEnough) 
    {
        if (payment == total)
        {
            isPaymentEnough = true;
        }
        else if (payment < total) 
        {
            Console.WriteLine
            (
                "Payment ${0:0.00} is not enough for total ${1:0.00}"
                ,payment, 
                total
            );
            payment = getPaymentFromCustomer(total);
        }
        else
        {
            isPaymentEnough = true;
            change = payment + -total;
            Console.WriteLine("Your change is ${0:0.00}.", change);
        }
    }
    return change;
}

To:

public static decimal processPayment(decimal total, decimal payment)
{
    while (true) 
    {
        if (payment < total) 
        {
            Console.WriteLine
            (
                "Payment ${0:0.00} is not enough for total ${1:0.00}"
                ,payment, 
                total
            );
            payment = getPaymentFromCustomer(total);
        }
        else
        {
            Console.WriteLine("Your change is ${0:0.00}.", payment - total);
            return payment - total;
        }
    }
}

The changes are:

  • I eliminated the boolean flag.
  • I avoided a temporary change
  • I returned from inside the loop to avoid break;
  • I eliminated an unnecessary special case.
  • From payment + -total to payment - total because the second is more intuitive.

Temporary return anti-pattern

You write:

public static decimal calculateTotal(Dictionary<string, int> order)
{
    decimal total = calculateSubTotal(order) + calculateSalesTax(order);
    return total;
}

Creating a temporary variable to return it a second after is overcomplicated, just return the value:

public static decimal calculateTotal(Dictionary<string, int> order)
{
    return calculateSubTotal(order) + calculateSalesTax(order);
}
\$\endgroup\$
3
  • \$\begingroup\$ Can you provide a concrete example using Linq? This would help a lot since this is very new to me, and I don't know anything about Linq. \$\endgroup\$ – Phrancis Dec 5 '15 at 22:01
  • \$\begingroup\$ @Phrancis Added an example, not tested, but should work. \$\endgroup\$ – Caridorc Dec 5 '15 at 22:04
  • \$\begingroup\$ @Phrancis you are very welcome :) \$\endgroup\$ – Caridorc Dec 6 '15 at 21:24
8
\$\begingroup\$

There has already been some good and valid points made in the other answers, read these carefully and try implementing them. In addition I would give some other pointers which I've stumbled upon when reading your code:

  • You shouldn't make it all static, as that cripples some of the code in general, so do change that as Hosch250 suggests! (Just had to repeat that one!)
  • It could be argued that to mimic closer the real life, you should have more objects in your program. One could argue that you should have a Customer and an Order classes at least. I'm just going to mention it, but that would allow for more coherent logic handling and easier extension on how to handle when multiple customers arrive, and possibly pay after being served, and extra orders, or extending the orders.
  • You lack documentation on how your restaraunt are meant to be used. No documentation in code, or in tests, or in public/protected/private suggestions. That last one is that all your methods are public, and so all of them should be able to be called at a seemingly random point in time for anyone having access to your class, and that isn't the intention is it?
  • Your class also holds very little on the current state of business. Everything gets shipped around in parameters. That is not the OOP way of doing business. To me it would be more natural if a restaurant class kept a record of all current customers with their corresponding orders, and have a public interface for entering customers, customers taking orders, customers paying their order, and customers leaving. In the code behind it would be natural to keep tabs on what has been ordered, how much money has been made, and ... You get the gist of it.
  • As you've written your code only one method should be public, and that is the processCustomer(), all else should be private. Within processCustomer() I would have separated the different parts even more clearly calling a sequence of the following methods:

    • Welcome customer
    • Take order
    • Present bill
    • Handle payment

    Most of this you've done already, but I would group some of the presentation and handling slightly different to make a coherent interface

  • Regarding calculation of the bill you do something strange. You calculate sub total, then you calculate tax, and then you calculate totals. Sounds good, but in calculate tax you recalculate sub totals, and in calculate total you recalculate taxes (which in turn recalculates sub totals) and sub totals. This is a lot of extra work... Make all of it into one method returning a tuples of the three values: subtotal, tax and total.
  • When accepting payments, something similar happens. You start of with getting a payment, goes to processing of payment, and if not enough you ask again for the total before looping. To me it would make more sense to ask for the remaining amount, as you most likely have been given some money already. (In real life, you wouldn't in most cases return the 20$ bill, and ask for more, you would ask for the remaining 4$. )
  • Finally, regarding overall logic, your business runs out of business after processing that one customer... You should problably allow for handling more than one customer.

Not so much actual code review here, but hopefully you have gotten a few pointers to how to improve your code to be more according to OOP. And again, do read the other answers which provide more direct coding suggestions. They are good advices!

Addendum: A little code on takeCateringOrder()

I see in the chat room there was a little discussion on how to do the purchasing of items, here is my stab at the code of the foreach loop:

int quantity;

while (True) 
{
   Console.WriteLine("Purchase how many of {0} for ${1:0.00} each?", product.Key, product.Value);

   var isValidQuantity = int.TryParse(Console.Readline(), out quantity);

   if (isValidQuantity && quantity >= 0)
   {
      break;
   }

   Console.WriteLine("Invalid order amount, enter 0 if you don't want any");
}

if (quantity > 0) {
   order.Add(product.Key, quantity);
}

This way your loop is clearly focused on getting a legal quantity, and breaks your when that has been achieved. Then it decides whether to add anything to the order or not.

\$\endgroup\$
1
  • \$\begingroup\$ That's some really sound advice overall about how to think of the code in a bigger picture, this is awesome and I'm definitely going to try and remember to apply this in code I write going forward! \$\endgroup\$ – Phrancis Dec 6 '15 at 21:26

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.