"Welcome to Buzzway Subs!"

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 cateringMenu = new Dictionary()
{
{"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,

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 _cateringMenu = new Dictionary()
{
    {"Sandwich Platter", 39.99M},
    {"Cookie Platter", 19.99M}
};

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

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

Ten, you can use `contin

StackExchange Code Review Q#113012, answer score: 17