C++ code for calculating the cost of carpet

Question

I have written the following code for the question (from Problem Solving With C++):

A store sells carpet for $2.75 per meter. If a customer buys more than 10m of carpet, they get a discount of 15% on every additional meter of carpet they purchase. Write a program that inputs the carpet length that a user wishes to buy, stores the value in a double variable, and then calculates and outputs the total cost of the carpet.

#include <iostream>
using namespace std;

int main()
{
    double cost_per_meter = 2.75;
    double length, extra_length, extra_length_cost, discount, 
    final_cost_of_extraLength, cost_of_10m, cost, total_cost;

    cout<<"\nEnter the length of carpet in meters that you wish to buy: ";
    cin>>length;
    
    if (length > 10)
    {
        extra_length = length - 10;
        extra_length_cost = extra_length * 2.75;
        discount = extra_length_cost * 15/100;
        final_cost_of_extraLength = extra_length_cost - discount;
        
        cost_of_10m = 10 * 2.75;
        total_cost = final_cost_of_extraLength + cost_of_10m;
        
        cout.setf(ios::fixed);
        cout.setf(ios::showpoint);
        cout.precision(2);
        
        cout<<"TOTAL COST = $"<<total_cost;
    }
    else
    {
        cost = length * 2.75;
        cout<<"Total cost "<<cost;
    }
    return 0;
}

As I am new to C++, I would like experts to review my above code for any improvements that could be made.

Answer 1

The code is longer and more complex than necessary. Here is what it could look like instead:

#include <iostream>
#include <iomanip>
#include <numeric>

int main()
{
    // Define the relevant constants
    static constexpr double cost_per_meter = 2.75;
    static constexpr double discount = 15.0 / 100.0;
    static constexpr double discount_start_length = 10.0;

    // Read the input length
    double length;
    std::cout << "Enter the length of carpet in meters that you wish to buy:\n";
    std::cin >> length;

    // Calculate the normal priced length and the discounted length
    double normal_length = std::min(length, discount_start_length);
    double extra_length = length - normal_length;

    // Calculate the total cost
    double total_cost = normal_length * cost_per_meter
                      + extra_length * cost_per_meter * (1 - discount);

    // Print the result
    std::cout.setf(std::ios::fixed);
    std::cout.precision(2);
    std::cout << "Total cost is $" << total_cost << '\n';
}

Here I've used the std::min() function to get the minimum of the input length and discount_start_length. By subtracting the result from the original length, we thus have normal_length which is capped to 10 meters, and we subtract this from the original length to get the same extra_length as you calculated. If length was less than 10 meters, then extra_length will be zero, so it will effectively not contribute to the total_cost. This avoids the if-statement and the code duplication. In particular, now there is only one place where the result is printed, and it is always done in the same way.

Answer 2

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

---

double cost_per_meter = 2.75;
Use const where you can! In this case, it should be constexpr since it is a compile-time constant.

---

double length, extra_length, extra_length_cost, discount, final_cost_of_extraLength, cost_of_10m, cost, total_cost;

You declared all the variables at the top of the function. Don't do that. Declare variables when first needed, where you are ready to initialize them.

---

 extra_length = length - 10;
 extra_length_cost = extra_length * 2.75;

See, here in an inner scope is where you actually use these variables. Define them here; e.g.

const auto extra_length = length - 10;

(remember to use const if you can). And, you seem to have completely forgotten to use cost_per_meter that you did define!

---

cout<<"TOTAL COST = $"<<total_cost;
   ⋮
cout<<"Total cost "<<cost;

You're printing in different ways (upper or lower case, = or not, $ or not) in two different places. The answer by G. Sliepen shows a better way to do it.

In general, you should only do things in one place. The code flow should re-combine after taking care of differences, so you only need one place to show the total.

You can see in his code that there is only one copy, and no if statements needed. If the discounted_length is zero then it still works in the full extended calculation.

He also didn't explain it, but specifying that the result is to be printed with exactly two decimal places in fixed (not scientific) format is important in the real world. Otherwise, you'll get output like "$3.2" when it would be normal to see "$3.20", or "$3.00000000097" when the correct answer was "$3.01" because such numbers cannot be represented exactly in floating point.

Answer 3

Based off of G. Sliepen's excellent answer, I would split the different sections of the program into functions. This makes the code much easier to read and maintain. It also removes the need for comments as the functions tell you exactly what they are doing. Comments tend to cause a lot of problems so it is much better to remove them with clean code where possible.

#include <iostream>
#include <iomanip>
#include <numeric>

double get_length_required() {
    double length;
    std::cout << "Enter the length of carpet in meters that you wish to buy:\n";
    std::cin >> length;
    return length;
}

double calculate_total_cost(const double length) {
    // Define the relevant constants
    static constexpr double cost_per_meter = 2.75;
    static constexpr double discount = 15.0 / 100.0;
    static constexpr double discount_start_length = 10.0;

    // Calculate the normal priced length and the discounted length
    double normal_length = std::min(length, discount_start_length);
    double discounted_length = length - normal_length;

    // Calculate the total cost
    return normal_length * cost_per_meter
                      + discounted_length * cost_per_meter * (1 - discount);
}

void print_result(const double total_cost) {
    std::cout.setf(std::ios::fixed);
    std::cout.precision(2);
    std::cout << "Total cost is $" << total_cost << '\n';
}

int main() {
    double length = get_length_required();
    double totalCost = calculate_total_cost(length);
    print_result(totalCost);
}

Run it online here.

This is just a short example. The code could be split into even smaller functions to make it easier to read. Putting things into classes would be the next step.

Answer 4

As it's not been pointed out already: NEVER EVER store anything that has to do with money as a floating point number (float or double). These types are not exact: they will create errors even at relatively small values and any math you do with them will not be correct to the precision that is needed for money.

Don't use floating point for money.

Instead use a tested and tried arbitrary precision decimal number library like http://gmplib.org/ or boost multiprecision.

Or if you're sure you only ever need to deal with small amounts and you're doing a small toy project or a game (basically not real money), use a long long (64 bit integer) to store everything as cents (so one dollar is stored as value 100). This is precise but needs some care in the implementation.

Answer 5

Separate mechanism from policy

You have done the right thing by defining cost_per_meter outside of the calculation - that's good (although you then seem to ignore that and hard-code the value anyway - not good).

We can improve by moving the other constants (the full-price length and the discount rate) to the same part of the code, because these are also quantities that might be changed according to the shop's whim in future.