patterncppMinor
Calculate total cost of items, based on bulk discount
Viewed 0 times
totalbulkitemscalculatebaseddiscountcost
Problem
I've written a short program for one of my first exercise in C++.... I would like to get better suggestions to solve it, so hope you can help me :) And I hope this is the right session to ask...:)
Exercise:
Input: Pieces and price per piece. If pieces > 10, its 5% discount. If pieces>50, 10% discount
Output: total price
Thank you a lot in advance
Exercise:
Input: Pieces and price per piece. If pieces > 10, its 5% discount. If pieces>50, 10% discount
Output: total price
Thank you a lot in advance
#include
#include
using namespace std;
int main() {
int stk, ep, gesamt;
cout > stk;
cout >ep;
if (stk>=10)
{
if (stk>=50)
//10% discount
{
gesamt=stk*ep;
gesamt=gesamt - (gesamt*10)/100;
goto stop;
}
else //5% discount
gesamt=stk*ep;
gesamt=gesamt - (gesamt*5)/100;
goto stop;
}
else gesamt=stk*ep;
stop:
cout << gesamt << endl;
return 0;
}Solution
Avoid
This can cause name collisions because it adds every name in the
Alternatively, you can introduce using declarations like
Avoid duplicated code like
That statement appears three times in your code. If that logic ever changes you'll need to make sure you change it everywhere, and you might miss one or more instances. Put that duplicated code in a function if necessary, or just write it once if possible.
Indent for readability
None of the code in
Use more whitespace for readability
E.g. use
Use descriptive variable names
The variable names seem to be in a different language (German?) but
Avoid
using namespace stdThis can cause name collisions because it adds every name in the
std namespace to the global namespace. For a small program like this one it's unlikely that you'll run into any problems (then again, maybe not) but it's best to get into the habit of using the std:: prefix on names in the std namespace.Alternatively, you can introduce using declarations like
using std::cout; to add specific names to the global namespace.Avoid duplicated code like
gesamt=stk*ep;That statement appears three times in your code. If that logic ever changes you'll need to make sure you change it everywhere, and you might miss one or more instances. Put that duplicated code in a function if necessary, or just write it once if possible.
Indent for readability
None of the code in
main() is indented, for example. It's easier to read your program if it's indented like this:int main() {
int stk, ep, gesamt;
//...
}Use more whitespace for readability
E.g. use
gesamt = stk ep; instead of gesamt=stkep;Use descriptive variable names
The variable names seem to be in a different language (German?) but
stk and ep in particular look like they are abbreviations or acronyms. I would prefer pieces for stk and unit_price for ep.Avoid
std::endl in favor of \nstd::endl flushes the stream, which can cause a loss in performance. In some cases you can also reduce calls to operator 10 but you use >=. Your code applies a discount if pieces = 10 but the problem statement implies that the discount should not be applied in that case (only starting with pieces = 11).
Use the appropriate type for variables
In this case, you probably need to use float or double for ep and gesamt. The price isn't necessarily an integer and neither is the total price if the discount is applied.
Check your inputs for error conditions
For example, neither the number of pieces nor the unit price should be negative. Check for these error conditions and handle them appropriately (output an error message, throw an exception, ask for a new input, etc.).
Avoid unnecessary goto statements
goto is generally not needed, and that is the case in your program. Your program will reach the stop statement after entering the various if/else clauses.
Result
Here's the end result of your program with the above suggestions (I've also made the code more concise in some cases, e.g. by the use of the *=` operator to multiply a number by itself):#include
int main() {
int pieces;
double unit_price;
double gesamt;
std::cout > pieces;
if (pieces > unit_price;
if (unit_price 10)
{
if (pieces > 50)
//10% discount
{
gesamt *= 0.9;
}
else //5% discount
{
gesamt *= 0.95;
}
}
std::cout << gesamt << '\n';
return 0;
}Code Snippets
int main() {
int stk, ep, gesamt;
//...
}#include <iostream>
int main() {
int pieces;
double unit_price;
double gesamt;
std::cout << "Pieces?\n";
std::cin >> pieces;
if (pieces < 0)
{
std::cout << "Cannot have negative pieces!\n";
return -1;
}
std::cout << "Price per Piece?\n";
std::cin >> unit_price;
if (unit_price < 0)
{
std::cout << "Cannot have negative unit price!\n";
return -1;
}
// Calculate total price
gesamt = pieces * unit_price;
// Apply discount, if applicable
if (pieces > 10)
{
if (pieces > 50)
//10% discount
{
gesamt *= 0.9;
}
else //5% discount
{
gesamt *= 0.95;
}
}
std::cout << gesamt << '\n';
return 0;
}Context
StackExchange Code Review Q#143120, answer score: 5
Revisions (0)
No revisions yet.