4
\$\begingroup\$

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

#include <iostream>
#include <stdio.h>
using namespace std;
int main() {
int stk, ep, gesamt;
cout << "Pieces?" << endl;
cin >> stk;
cout << "Price per Piece?" << endl;
cin >>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;
}
\$\endgroup\$
5
\$\begingroup\$

Avoid using namespace std

This 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=stk*ep;

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 \n

std::endl flushes the stream, which can cause a loss in performance. In some cases you can also reduce calls to operator<< (e.g. cout << "Pieces?" << endl; becomes cout << "Pieces?\n";).

Check the problem statement logic

The problem statement says to apply a 5% discount if pieces > 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 <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;
}
\$\endgroup\$
4
\$\begingroup\$

Missing braces leads to use of goto

Your code here is missing braces for the else case:

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;

The only reason this code works is because you have a goto stop in the first case. If you just used braces properly, you wouldn't need those goto statements. Also, you can also get rid of the outer if statement and end up with this:

gesamt=stk*ep;
if (stk > 50)
{
    //10% discount
    gesamt -= (gesamt*10)/100;
}
else if (stk > 10)
{
    //5% discount
    gesamt -= (gesamt*5)/100;
}
cout << gesamt << endl;
\$\endgroup\$

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.