Calculating Discount C++

Question

The following question is from the book Problem Solving With C++. This is given as an exercise in the chapter "Functions for all subtasks":

Write a function that calculates a discount applicable on the price of an item. Your function should have three arguments: the price as a reference to a double, the discount as a double value, and a bool to indicate if the discount is calculated as a percentage or a fixed amount. You should calculate the discount and modify the price of the item accordingly. Your program should check that the discount is not negative, and that the price of the item does not drop to zero after applying discount. Use assert statement for this.

Here's my solution:

#include<iostream>
#include<cassert>

double calcDiscount(double& price, double discount, bool type);

int main()
{ 
  char type;
  double price, discount, disAmt;
  std::cout<<"Enter price of the item = ";
  std::cin>>price;
  disAmt = calcDiscount(price, discount, type);
  price = price - disAmt;
  assert(price > 0);
  std::cout<<"Price after discount = "<<price<<"\n";
}

double calcDiscount(double& price, double discount, bool type)
{
  std::cout<<"Enter 1 to calculate discout as percentage.\n"
           <<"OR\n"
           <<"Enter 0 to calculate discount as fixed amount.\n";
  std::cin>>type;
  if(type)
  {
    std::cout<<"Enter discount percentage = ";
    std::cin>>discount;
    discount = (price * (discount/100.0));
  }
  else
  {
    std::cout<<"Enter discount amount = ";
    std::cin>>discount;
  }
  assert(discount >= 0);
  return(discount);
}

Although it gives the correct output. But still what improvements could be made?

Answer 1

You should put main at the bottom. It's not useful to declare your functions in a CPP file, since C++ supports overloading. If you defined the actual function with a different signature that would not be a compile-time error. But you would have a linker error -- the pre-declaring of the function did not help you, and only caused more confusion.

So, put functions in the order they need to be. Declarations in the CPP files are only for mutual recursion.

---

So much of what's poor with the solution is due to the specification! I understand that a beginner can't swallow everything at once, and it's trying to teach functions and decomposition in this lesson.

Your function should have three arguments: the price as a reference to a double, ...

Never never never use double for money.
I was shocked at how many pros in business did not learn this. So don't flirt with such an insidious habit even in toy code.

And making it a reference is suspicious.

...the discount as a double value, ...

That seems like it might be OK for a rate, but it runs into some of the same issues. E.g. you can't represent 10% exactly.

...and a bool to indicate if the discount is calculated as a percentage or a fixed amount.

You're using the same type to represent an amount or a discount. Are they the same type of thing? Even if they can be represented by the same underlying primitive type, they are not the same kind of thing. You're writing code using primitives in the declaration, not domain-specific types. You are not passing a double but an Amount, logically. A book that teaches problem solving should introduce that idea early, too.

As for using a bool to indicate two modes of operation for the function: Well, that does happen. A bool can be OK here, but only just. It's really poor design to have two different functions under one wrapper, without having a majority of the code in common.

BTW, I happen to have exactly that in my own code! But, it's not passing a amount-or-rate as a single parameter. The amount, or the rate, is found in a database. In my case, I use an enum to indicate Rate or Discount (or whatever I called them). And the function in question called the underlying calculation as one line, and the rest of the function was doing work that didn't matter which it was, so it's sensible to combine them into one function like that.

You should calculate the discount and modify the price of the item accordingly.

Don't use "in-out" parameters! This is poor design. It should be returning the new value. You are returning double too even though this spec does not say anything about a return value.

Your program should check that the discount is not negative, and that the price of the item does not drop to zero after applying discount. Use assert statement for this.

Checking is good. The assert statement, not so much. It only has an effect in a debug build and is ignored in a release build! It's difficult to unit test that your code performs that check, as it simply aborts the program. An exception would be better. There is an exception type designed for exactly this.

---

Are the requirements clear?

You write (price * (discount/100.0)); interpreting the discount parameter as 30.0 meaning 30%. But it's a double, and it's normal to use a floating-point value to mean a value between 0.0 and 1.0. That is, 30% would naturally be 0.3.

---

Jarod42 already pointed out that you are asking for the type parameter inside the function. You are not passing it to the function! In main you declare it as char (not bool) but don't use it.

You are missing the point of having a function. You should collect the needed information and hand it all to the function. The function should look only at its parameters and not worry about where the information came from.

Then you prompt for discount! You are completely missing the point of using a function that takes discount as a parameter!

---

Worse, you are not computing the discount amount in the non-percent case. You are neither computing the amount nor checking for a negative value as directed. Your code gives the wrong answer and fails.

You should not post on Code Review until code is complete and working correctly. You don't even know... this shows why you need testing. One big advantage of using functions (correctly) is you can call them from test code with built-in values which are checked automatically, not prompting you for anything at all. This lets the program auto-test the logic without you having to repeatedly type in values.

---

You write: return(discount); But return is not a function. Write return discount; without redundant parens around the whole thing. This does make a difference in some cases as of C++11.

---

Don't Give Up

This is the way to learn these things. The books don't tell you everything, and trying things for yourself is the only way to figure it out.

Answer 2

Sorry as expected function interface is bad (having bool parameter instead of 2 functions, mixing in/out parameter and return value, function doing several stuff with naming not reflecting that, ...).

All I/O should not be in calcDiscount:

double calcDiscount(double& price, double discount, bool as_percentage)
{
    assert(discount >= 0);
    if (as_percentage) {
        discount = price * discount / 100.0;
    }
    price -= discount;
    assert(price > 0);
    return discount;
}

Then in main, you might do the I/O

int main()
{ 
  double price;
  std::cout << "Enter price of the item = ";
  char type;
  std::cin >> price;
  std::cout << "Enter 1 to calculate discount as percentage.\n"
            << "OR\n"
            << "Enter 0 to calculate discount as fixed amount.\n";
  std::cin >> type;
  double effective_discount;
  if (type == '1') {
    std::cout << "Enter discount percentage = ";
    double discountPercent;
    std::cin >> discountPercent;
    effective_discount = calcDiscount(price, discountPercent, true);
  } else {
    std::cout << "Enter discount amount = ";
    double discount;
    std::cin >> discount;
    effective_discount = calcDiscount(price, discount, false);
  }
  std::cout << "discount = " << effective_discount << "\n";
  std::cout << "Price after discount = " << price << "\n";
}