"The Trip" expense-equalizing challenge [closed]

Question

I've solved The trip from Programming Challenges. My solution suffers from lack of floating-point precision, though.

The problem:

Your job is to compute, from a list of expenses, the minimum amount of money that must change hands in order to equalize (within a cent) all the students' costs. Input Standard input will contain the information for several trips. The information for each trip consists of a line containing a positive integer, n , the number of students on the trip, followed by n lines of input, each containing the amount, in dollars and cents, spent by a student. There are no more than 1000 students and no student spent more than $10,000.00. A single line containing 0 follows the information for the last trip. Output For each trip, output a line stating the total amount of money, in dollars and cents, that must be exchanged to equalize the students' costs.

For example, if you have:

4
15.00
15.01
3.00
3.01

The output should be:

$11.99

My code:

#include <iostream>
#include <iomanip>

using namespace std;

float two_precision(float n){
    return n - float((n*100.0-(int)(n*100))/100.0);
}

int main(){

  int n; cin >> n;
  float *v, sum, avg, dif;

  while(n){

    sum = dif = 0.0;
    v = new float[n];
    for(int i = n-1; i >= 0; i--){
      cin >> v[i];
      sum += v[i];
    }

    avg = (1.0*sum)/n;

    for(int i = n-1; i >= 0; i--)
      if(two_precision(v[i]-avg > 0))
        dif = two_precision(dif + two_precision(v[i]-avg));

    cout << '$';
    cout << fixed << setprecision(2) << dif << endl;

  delete v;
  cin >> n;
  }

return 0;
}

I have one case to show the error:

5
15.04
15.17
30.32
39.99
78.99
0

The output should be $47.17 and my code returns $47.16.

Answer 1

What you need is a Money class.

class Money
{
    int   value;
    static int normalize(double dallarsAndCents)
    {
        return std::round(dallarsAndCents*100);
    }
    public:
        // Deliberately not using explicit (because I want double;s to auto convert)
        Money(double dallarsAndCents)
           : value(normalize(dallarsAndCents))
        {}

        friend std::ostream& operator<<(std::ostream& out, Money const& m)
        {
            return out << m.value / 100 << "." << std::setw(2) << std::setfill('0') << m.value % 100;
        }
        friend std::istream& operator<<(std::istream& in, Money& m)
        {
            double newValue;
            if (in >> newValue) {  // only do assignment if read works.
                m = Money(newValue); 
            }
            return in;
        }


        // Define some arithmetic operations here.

};

You don't want to use new or delete in your code. If you are doing that (and you are not in the very bottom of a class doing resource management) then you are doing it wrong.

Local variables just declare locally. If you need a dyncmaically sized container of objects then use a standard container (std::vector is a good default to start with).

v = new float[n];
....
delete v;

This looks neat and tidy but is not good code. You are susceptible to a memory leak if an exception is thrown. You should be using a container that handles the memory for you.

 std::vector<float>  v(n); // Does the same thing but is exception safe
                           // Look up RIAA for why it is exception safe.

Don't use one line for multiple things.

int n; cin >> n;

This is just lazy. It reads nicer if you use more than one line.

Also read Why is “using namespace std;” considered bad practice? about using namespace;. The reason the standard library is named std and not standard is so that prefixing things is not a major burden. Prefer to use std::cin.

Again the one line thing.

float *v, sum, avg, dif;

It makes it hard to read. The * is on the wrong side because you are squishing things together. Note: The star is part of the type information not the object. This is the opposite of C style.

float*  v;
float   sum = 0.0;
float   avg = 0.0;
float   dif = 0.0;