1

I am a c++ rookie and do not know what I am doing wrong. My assignment is to compare two different .txt files, each containing an item along with either the number of items and the price of that item. I am then trying to print the names and prices of the items. Lets say I am using the .txt file namesAndQuantity.txt which includes:

3 books
4 pens

And a .txt file namesAndPrice.txt which includes:

pens 3.45
books 19.55

The code I am using only prints out the first match:

#include <iostream>
#include <fstream>
#include <cmath>

int main(){
    string nameOfItemP, nameOfItemQ;
    double priceOfItem;
    int numberOfItems;
    ifstream inData;
    ifstream inData2;
    inData.open("namesAndQuantity.txt");
    inData2.open("namesAndPrice.txt");
    while (inData>>numberOfItems>>nameOfItemQ){
        while (inData2>>nameOfItemP>>priceOfItem){
            if (nameOfItemP==nameOfItemQ){
               cout<<nameOfItemQ<<endl;
               cout<<priceOfItem;
        }   
    }
}

This code only prints out the first line:

books
19.55

What can I do to better it?

John
  • 13
  • 1
  • 3

3 Answers3

2

It's because after the first

while (inData2>>nameOfItemP>>priceOfItem){
    if (nameOfItemP==nameOfItemQ){
        cout<<nameOfItemQ<<endl;
        cout<<priceOfItem;
    }   

executes, inData2 reaches its end and will not read any more. The solution is to move the open function into the while loop:

while (inData>>numberOfItems>>nameOfItemQ){
    inData2.open("namesAndPrice.txt");
    while (inData2>>nameOfItemP>>priceOfItem){
        if (nameOfItemP==nameOfItemQ){
           cout<<nameOfItemQ<<endl;
           cout<<priceOfItem;
    }
    inData2.close();  
}

However, this is not the best approach. You'd better use a map to avoid nested loops. A map is like an array except that you can choose to use a string as an index:

#include <iostream>
#include <fstream>
#include <string>
#include <map>

using namespace std;

int main() {
    // same as before
    int numberOfItems;
    string nameOfItem;
    double price;

    // create a map, using string as index and int as value.
    map<string, int> items;

    ifstream inData("namesAndQuantity.txt");
    ifstream inData2("namesAndPrice.txt");

    while (inData >> numberOfItems >> nameOfItem)
      items[nameOfItem] = numberOfItems;

    while (inData2 >> nameOfItem >> price)
      cout << nameOfItem << " "
           << items[nameOfItem] << " " << price << endl;

    inData.close();
    inData2.close();

    return 0;
}

Output

pens 4 3.45
books 3 19.55
Yang
  • 7,712
  • 9
  • 48
  • 65
0

You have two while loops, The first one is okay, however the second while loop will terminate after executing the first priceofItem look up. It will traverse the entire file to end for the first lookup, And when the next iteration try to execute from the outer while loop, the inner loop is FALSE, its like while(false). What you need is a storage, An Arraylist, that will store the names and price of item. This will provide multiple iteration from I = 0, to I = n.

Juniar
  • 1,269
  • 1
  • 15
  • 24
0

I think @Yang has at least some of the right general ideas, but I think I'd do things a little bit differently. His code is only good for reading the data and displaying it exactly as prescribed here. I'd rather see some code that has a little more general purpose applicability, such as storing all the data so if (for example) you wanted to print it in a different format or filter it before printing, that would be somewhat more practical. With that in mind, I'd write the code a bit more like this:

#include <iostream>
#include <string>
#include <map>
#include <algorithm>
#include <vector>
#include <iterator>
#include <fstream>

struct item {
    int quantity;
    double p;

    friend std::ostream &operator<<(std::ostream &os, item const &j) {
        return os << "\t(" << j.quantity << ")\t$" << j.p;
    }
};

std::ostream &operator<<(std::ostream &os, std::pair<std::string, item> const &r) {
    return os << r.first << ":" << r.second;
}

void read_quantities(std::string const &fname, std::map<std::string, item> &items) {
    std::ifstream in{ fname };
    std::string name;
    int temp;
    while (in >> temp >> name)
        items[name].quantity = temp;
}

void read_prices(std::string const &fname, std::map<std::string, item> &items) {
    std::ifstream in{fname};
    std::string name;
    double temp;
    while (in >> name >> temp)
        items[name].p = temp;
}

int main() {
    std::map<std::string, item> items;

    read_quantities("quantities.txt", items);
    read_prices("prices.txt", items);

    for (auto const & item : items)
        std::cout << item << "\n";
}

I also prefer to maintain some separation of concerns -- a function should generally have a single...function. I prefer, for example, for code that reads to only read, and code that writes to only write, rather than having a single function the takes some existing data, reads more data from a file, then writes out a combination of the existing data with the new data.

That's not to say the latter is horrible or awful or anything like that -- just that I'd prefer to restrict each function to doing one thing when reasonable.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111