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.

• 001: You might want to check for bad input: if (!(std::cin >> length)) { ... }
• JPhi1618: DIY.SE user checking in here... Carpet is typically sold by the square meter. It doesn't change your code - I just don't want you to be disillusioned if you try to estimate the carpet cost for your house!

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.

• L. F.: std::min is defined in <algorithm> instead of <numeric>. Did you mean to #include the former?
• David: @G. Sliepen I reused your code to give an example using functions. I'll delete / rewrite it if you would like.
• G. Sliepen: @David It's totally fine!
• Strange Alchemist: thanks @G.Sliepen but as a total newbie to C++ I right now don't know about static constexper and std::min . Also I have only used iostream header file so far.
• G. Sliepen: No problem, we all start small. The standard library provides a lot of useful functions though, and more experienced C++ programmers know when to use them. To get that experience yourself, have a look at cppreference.com and this list of C++ books, and of course keep coding :)

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.

• G. Sliepen: OP did print with fixed decimals in the first part of the if-statement, just not in the else-part. Anyway, thanks for the more detailed explainations, I skipped that in my answer.

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.

• IceGlasses: Emphasis: It's the decimal (i.e. base 10) nature rather than the arbitrary precision that's important here.
• Emily L.: If FP precision and accuracy would be sufficient, then you could correctly correctly represent base 10 decimals and numbers. It's just that many numbers are not accurately representable by floating point, for example 0.1 cannot be exactly represented with a finite number of bits (for the same reason 1/3 can't be exactly represented in decimal with a finite number of digits). Also issues caused by truncation such as catastrophic cancellation are also related to precision and a major cause for the recommendation. Decimal is one trigger of the cause that is precision.
• Toby Speight: @IceGlasses, it's both the radix and the precision that can cause problems. That said, it's unlikely anyone will be buying enough carpet that floating-point won't get integers right (and in the particular example, we have 2.75 $/m, which is exactly representable in both binary and decimal floating-point, but let's not dismiss the lesson just for that!)

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.