The code outputs the value wrongly

[marialavr]

I am trying to write a code that will print the number values and tat the number of the smallest number of coins that I can exchange my money with. However, If I compile it for the first time, it works normally, but it doesn't if I am re-using it over and over again.

Next I've tried to create array instead of creating vector of values and it works perfectly, could you explain what's wrong with this code.

Thanks in advance.

#include <iostream>
#include <vector>
#include <cstring>
#include <algorithm>

using namespace std; 

//All current denominations
std::vector<int> values = {1, 5, 10};
int n = values.size();

//Finding minimum number of coins 
void findMin(int sum) {
    vector<int> coins; 
    for (int i = n; i >= 0; --i ) {
        while (sum >= values[i]) {
            sum -= values[i];
            coins.push_back(values[i]);
        }
    }
    for (int i = 0; i < coins.size(); i++) {
        cout << coins[i] << "\t";
    }
    cout << coins.size();
}

int main() {
    int sum; 
    cin >> sum; 
    cout << " The minimum number of coins noted that sum up to " << sum << "\t";
    findMin(sum);
    return 0;
}

[George P]

#include <iostream>
#include <vector>
#include <string>

int main()
{
   // create a container with coin/bill denominations
   std::vector<int> denominations { 100, 25, 10, 5, 1 };
   std::vector<std::string> denom_names { "Dollar(s)", "Quarter(s)", "Dime(s)", "Nickel(s)", "Pennies" };

   // create a container to hold the number of bills/coins
   // the same size as the number of denominations
   // zero-filled by default
   std::vector<int> change(denominations.size());

   // let's ask for an amount of money to calculate
   std::cout << "How much money do you have? ";
   double d_amount;
   std::cin >> d_amount;
   std::cout << '\n';

   std::cout << "You entered $" << d_amount << "\n\n";

   // let's integerize the decimal amount
   int amount = d_amount * 100;

   // let's loop through all the demonination values
   // calculating the number of each from the amount
   for (size_t loop { }; loop < denominations.size(); loop++)
   {
      change[loop] = amount / denominations[loop];

      // get the remainder
      amount %= denominations[loop];
   }

   int sum { };
   std::cout << "That is:\n";

   for (size_t loop { }; loop < denominations.size(); loop++)
   {
      std::cout << change[loop] << ' ' << denom_names[loop] << '\n';
      sum += change[loop];
   }
   std::cout << '\n';

   std::cout << "The minimum number of bills/coins is " << sum << '\n';
}

How much money do you have? 3.68

You entered $3.68

That is:
3 Dollar(s)
2 Quarter(s)
1 Dime(s)
1 Nickel(s)
3 Pennies

The minimum number of bills/coins is 10

How much money do you have? 1.50

You entered $1.5

That is:
1 Dollar(s)
2 Quarter(s)
0 Dime(s)
0 Nickel(s)
0 Pennies

The minimum number of bills/coins is 3

[marialavr]

But what is exactly my mistake here? Cause from time to time it outputs correctly.

[George P]

Your logic for calculating each type of coinage is flawed. That you occasionally get a correct answer doesn't change that.

Take advantage of what integer division (and modulus) can do for you, no need for a while loop to get the number of coins for each denomination.

Looping from highest coin value to lowest makes / and % easier to work with.

[marialavr]

Thank you a lot for your answer. I will definitely follow your advice, but just to make it a bit clearer for me - could you point in my code where the mistaken hides?

[George P]

Lines 15- 23 is where the beastie seems to lurk.

The lines where you calculate the number of each type of coin.

Without debugging your code, monitoring the values of your variables through each loop, for and while, it is hard to say exactly what is going wrong.

Do you have a number you know produces a wrong output? The few numbers I try (after commenting out line 22) show the total value and then the number of coins. Every number I try gives correct output.

[againtry]

Looks like there are just a couple of changes required. Reversing the order of the vector just made it all that easier.

#include <iostream>
#include <vector>
#include <cstring>
#include <algorithm>

using namespace std;

//All current denominations
std::vector<int> values = {10, 5, 1};
int n = values.size();

//Finding minimum number of coins
void findMin(int sum){
    vector<int> coins;
    for (int i = 0; i < n; i++ ) {
        while (sum >= values[i]) {
            sum -= values[i];
            coins.push_back(values[i]);
        }
    }
    for (int i = 0; i < coins.size(); i++)
    {
        cout << coins[i] << "\t";
    }
    cout << '\n';
    cout << "No. of coins: " << coins.size() << '\n';
}

int main() {
    int sum;
    cout << "Enter sum: ";
    cin >> sum;
    cout << " The minimum number of coins noted that sum up to " << sum << "\t";
    findMin(sum);
    return 0;
}

[againtry]

Or we'll do it your way:

#include <iostream>
#include <vector>
#include <cstring>
#include <algorithm>

using namespace std;

//All current denominations
std::vector<int> values = {1, 5, 10};
int n = values.size() - 1;

//Finding minimum number of coins
void findMin(int sum){
    vector<int> coins;
    for (int i = n; i >= 0; i-- ) {
        while (sum >= values[i]) {
            sum -= values[i];
            coins.push_back(values[i]);
        }
    }
    for (int i = 0; i < coins.size(); i++)
    {
        cout << coins[i] << "\t";
    }
    cout << '\n';
    cout << "No. of coins: " << coins.size() << '\n';
}

int main() {
    int sum;
    cout << "Enter sum: ";
    cin >> sum;
    cout << " The minimum number of coins noted that sum up to " << sum << "\t";
    findMin(sum);
    return 0;
}

[Handy Andy]

Hello marialavr,

To more directly answer your question.

"<cstring>" should be "string", but even then it is not needed for this program.

"<algorithm>" is not needed at all or is not needed yet.

For line 6:

//using namespace std;  // <--- Best not to use.
// A recent post that is worth reading. http://www.cplusplus.com/forum/beginner/258335/ 

Chances are that this is what you were taught and told to use, but it will become a problem some day. An alternative that is better is:

using std::cin;
using std::cout;
using std::endl;
using std::vector;

You will find this is a more narrow approach for what you need.

Lines 9 and 10 as a global variables should be avoided. The only place they are used is the function, so I would put them there.

For line 9 I would take the hint from Furry Guy and add at least 25 to the vector.

For line 10 understand that the ".size()" and ".length()" functions return a number of "size_t", AKA "unsigned int", and that storing this value type into an "int" there could be possible data loss because an "unsigned int" can store a number larger than what an "int" can. This should generate at least a warning from the compiles, but will not stop the program from running.

Also the ".size()" and ".length()" functions return the total number of elements that are in, in this case, the vector. In C++ and some other languages things like arrays and vecctors along with other containers are (0) zero based, so the elements go from (0) zero to 2. This will be better understood shortly.

The variable name of "n" may be easy to type, but only have meaning to you. For some people it becomes a problem trying to keep track of what it is for. A suggestion call the variable "valuesSize". At least you have a better idea of what it is for.

In the function the for loop would start to look better as: for (size_t i = valuesSize; i >= 0; i--). Now you can understand better that "valuesSize" has a value of (3) then when you look at while (sum >= values[i]) you can realize that the vector does not have an element numbered (3) since it goes from (0) zero to (2). In the end the for loop should be for (size_t i = valuesSize - 1; i >= 0; i--) or take the suggestion from againtry int n = values.size() - 1; or more proper size_t valuesSize = values.size() - 1;.

After that a couple of "\n"s will improve the output on the screen.

In main I will start with a few blank lines makes the code easier to read and follow.

int main()
{
	int sum;

	cin >> sum;

	cout << " The minimum number of coins noted that sum up to " << sum << "\t";

	findMin(sum);

	return 0;
}

Your use of the {}s is totally your choice and what you are comfortable with. With whatever you choose just be consistent in its use. You should take a look at https://en.wikipedia.org/wiki/Indentation_style This will show you several different styles of using the {}s that you can compare.

The biggest point is to make you code easy to read and understand. Personally I like the "Allman" style, but it is your choice.

The next problems has to do with line 5 in the above code. When the program runs you are staring at a blank screen wondering what to do. The "cin" statement needs a prompt to tell the user what to enter.

Look at againtry's first or second code line 31 or Furry Guy's code line 17 for a suggestion.

Other than the blank lines the rest of "main" works.

Hope that helps,

Andy

[Handy Andy]

Hello marialavr,

One thing I forgot to mention.

Be careful using the "\t". This does not always space things the way that you want or line up columns properly.

Sometimes it is better to use spaces or "setw()" from the "<iomanip>" header file.

Andy