3
\$\begingroup\$

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.

\$\endgroup\$

closed as off-topic by Hosch250, Jerry Coffin, mdfst13, SirPython, Pimgd Feb 13 '16 at 0:24

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Hosch250, Jerry Coffin, mdfst13, SirPython, Pimgd
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Welcome to Code Review! Unfortunately, your question is off topic because we review and improve working code, rather than fixing bugs in code. Once you get it working, we will be glad to help you improve it. Tip - don't use double for money ever. In C++, you typically use an integer: stackoverflow.com/a/149365 \$\endgroup\$ – Hosch250 Feb 12 '16 at 22:00
  • 4
    \$\begingroup\$ I consider this solution to be substantially correct. It is closer in nature to seeking improvement than hunting for a logic error. \$\endgroup\$ – 200_success Feb 12 '16 at 22:37
  • 1
    \$\begingroup\$ Don't use floating point numbers when dealing with money. Multiply all numbers by 100 and then treat them as integers (round to the nearest value; though floating pointers are not exact they will be very close to the actual value so rounding the nearest after multiplying by 100 will give you the correct number of cents). That will remove your rounding issues. Only put the decimal point in when printing the result. \$\endgroup\$ – Martin York Feb 12 '16 at 23:00
  • \$\begingroup\$ I tried using just integers and I have the same problem. \$\endgroup\$ – HiHello Feb 13 '16 at 20:15
  • \$\begingroup\$ I edited the question including the code using int. \$\endgroup\$ – HiHello Feb 13 '16 at 20:19
2
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advises. But, in practice, I'll never declare a class like that if I'm competing. \$\endgroup\$ – HiHello Feb 13 '16 at 20:20
  • 1
    \$\begingroup\$ @ZebraDoMal Competing for what exactly? What is the goal, to implement code very quickly or cleanly? \$\endgroup\$ – Simon Forsberg Feb 15 '16 at 12:12
  • \$\begingroup\$ Quickly, just.. \$\endgroup\$ – HiHello Feb 17 '16 at 21:44

Not the answer you're looking for? Browse other questions tagged or ask your own question.