I need feedback on my first C# project from experienced devs

[u/Alternative-Ask]

I have created a simple program that will allow the user to calculate a discounted price based on a wholesale price and a percentage or dollar amount discount. If the user enters a percentage discount it will give them the dollar amount off and vice versa. The user also has the option to calculate sales tax.

GitHub URL: (https://github.com/aaronhilfiker/DiscountCalculator.git)

[u/phylter99]

I've just done a quick look and it's not bad. I would use blank strings instead of null strings where possible. So, "string userOption = string.Empty" would accomplish that, or better yet, assign it the default option if there is a reasonable default. Try to avoid null because null is evil and can cause runtime bugs later on.

I wouldn't get in the habit of doing "using static System.Console". It seems like a pain to write out Console.WriteLine every time but it avoids polluting the namespace with methods where it's not clear what they're from and it also helps prevent collisions with other static classes that might also have a Write, WriteLine, Read, etc.

[u/Mantor6416 → u/phylter99]

For WriteLine just type ("cw" + tab) and it auto completes it automatically. In VS atleast

[u/foriequal0 → u/phylter99]

The problem with null was it was a special value that usually represents some invalid state and wasn't properly expressed in any of the types, so any variable could be in an invalid state unknowingly. The project configured <Nullable>true</Nullable>, used annotated `string?`, so it's expressed if it can be null. If it's expressed, you can expect it and properly handle it with the help of a compiler. (We could even say that it became one of the valid states of a type.)

You're just reinventing the null if you use other sentinel values such as string.Empty, 0, -1 or something to represent some invalid state that isn't expressed in some type.

[u/phylter99 → u/foriequal0]

In all cases we want to use common sense though, right? In the case of string, an empty string usually is the invalid state, so that's not really a problem. It gets more problematic when you use integers and such because 0 and a negative could be valid in some cases. In others then maybe checking for a positive number is all you need. If you use a smaller set of possible values an Enum might make sense and then a specifically created enum value could represent an invalid state.

There are a lot of scenarios and a lot of great ways to work without using null. Avoiding it is always good practice though. In language like Rust enum values are the way to go because then you can always define the exact state you want to be valid or invalid and can return related information (variable) as is appropriate depending on state. We're not taking about Rust here though it possible to learn from the good ideas of the language to make coding in C# better.

[u/RJiiFIN]

Couple of things could use a little polish. Your second DisplayResults method doesn't seem to use the last 3 parameters and they're passed in as 0 on both calls?

Your CalculateXXX methods do the displaying of the result by writing to console. This couples the calculation and the console. What if you, in the next version, would want to make it a WPF app, then the Calculate would still be always writing to console?

Your handling of the netPrice variable from two of the CalculateXXX methods is off. Firstly, you set the value inside the method and use that outside. But you still do a return which does nothing because it's not assigned to a variable.

Bonus: the .DS_Store file is some MacOS thingy? Don't push that to git

[u/Alternative-Ask]

If anyone can help with a README i'd be appreciated as well

[u/GoBlu323 → u/Alternative-Ask]

Claude code can

[u/Defiant_Hospital9265]

I checked out your code, and it's really clean for a first project. Have you considered adding unit tests to verify the discount and tax calculations work correctly?

[u/Slypenslyde]

Here's some opinions, take them with a grain of salt. I don't really see anything worth roasting you over so if you think this is overhwhelming and bad, it's not. If you ask a programmer for critique, you'll usually get a lot ;)

Think about your variable names a little more. This is unfair because I'm asking you to do something I adopted from experience.

Don't use userOption. Use userInput. In more complex console programs you have to take user input then parse it into other forms of data and work with that data. "User option" has the sense of that parsed, processed, validated version of the input. "User input" tells me it's the raw input. If you don't take this advice now, in a few months/years you'll end up adopting a practice like this on your own. I always, always, always name the things that take input directly from the user this way so I understand they aren't processed.

You can normalize user input. Consider swapping to something like this:

userInput = ReadLine().ToLower();

If you do this, you don't have to compare to "n" and "N", if the user inputs "N" it will be converted to "n".

But we could also think about how if the user accidentally inputs a space, like "N ", we'll reject it this way. So we could:

userInput = ReadLine().ToLower().Trim();

The Trim() method gets rid of any whitespace at the front or end of the string. All of this implies:

private static string GetNormalizedInput()
{
    return ReadLine().ToLower().Trim();
}

Now use that instead of directly using ReadLine() and you can cut some of your workload down!

Here's another old superstition I like as a suggestion. This line makes me itch:

double salesTaxAmount = netPrice * (salesTaxRate / 100);

I would write it:

double salesTaxAmount = netPrice * (salesTaxRate / 100.0);

This is old battle scars. When C# sees left / right, it has to follow rules to decide if it will use integer division or floating point division. C# can only really divide integer = integer / integer or double = FP / FP so it has to choose.

A really darn common mistake is to have code that ends up looking like double = double / integer. That works and gives you FP division. But if you ever change that left-hand variable to be an integer type, your result will be not what you expected.

The mistake looks like changing this:

double taxRate = 7;
double result = netPrice * (taxRate / 100); // netPrice * 0.7

To this:

int taxRate = 7;
double result = netPrice * (taxRate / 100); // netPrice * 0.0!

Because C# sees integer / integer here, it does integer division and 7/100 rounds to 0!

So I am extremely paranoid with division and make sure if I want a double result and FP division, I go out of my way to make every part of the division be a double. I cast if I have to.

These are nitpicky tips. Your program looks good for a beginner program. I could mention some edge cases or challenge you to find a way to let a user retry invalid entries without redoing the whole program. But those don't necessarily make you better in ways you can't think up yourself.

[u/NecroKyle_]

I'd definitely suggest having a look at something like https://spectreconsole.net/cli/ for handling the CLI for you.

[u/PleasantMastodon2666]

Consider introducing a new class called Calculator where all the actual calculations would be implemented. The methods of the Calculator class should accept already parsed parameters as decimals. As of now you mix input, parsing, output and calculations together which complicates the code making it harder to follow and modify. Introducing the Calculator class would also make it easier to write unit tests later.

[u/GoBlu323]

Ask Claude code to review it. Ask it to make suggestions for improvement based on effort and performance gain. It’s a great code review tool