Reading in data from a file and then assigning vals and then creating object

So I have some code which I created from the top of my head. But in all honesty I know it can't be the most efficient way of doing it.

I have some data in a file all positioned 5 values per line separated by commas (one line will be a specific object) dependent on the name of the item.

I know this can't be the best way of doing it as I have done the exact same code in java with less than half the lines.

PS: I started learning C++ this week, so don't be too hard on me!

Here is my code:

```
ifstream inFile;
inFile.open("inventory.txt");

// check for error
if(inFile.fail()){
cerr << "Error opening file" << endl;
exit(1);
}

InventoryLinkedList list;
string line;
int counter =0;
while(getline(inFile, line)){
stringstream linestream(line);
string value;
int i = 0;
string type;
string code;
int count;
int price;
string other;
while(getline(linestream,value,',')){
if(i==0){
cout<< "Type " << value << endl;
type = value;
}
else if(i==1){
cout<< "Code " << value << endl;
code = value;
}
else if (i==2){
cout << "Count " << value << endl;
count = atoi(value.c_str());
}
else if (i ==3){
cout << "Price " << value << endl;
price = atoi(value.c_str());
}
else if(i ==4){
cout << "Other " << value << "\n"<< endl;
other = value;
}
i++;
if(i ==5){
StockItem *t;
if(type == "transistor"){
t = new Transistor(code,count,price,other);
}else if (type == "IC"){
t = new IC(code,count,price,other);
}else if (type == "resistor"){
t = new Resistor(code,count,price,other);
}else if (type == "capacitor"){
t = new Capacitor(code,count,price,other);
}else{
t = new Diode(code,count,price,o

Code Review

You don't check for failures in the read. When you read from a stream it can fail. All the read operations return a stream object as a result. This allows two things. 1) Chaining of read operations 2) Ability to check if it works. When you use a stream object in a boolean context it converts itself to bool that indicates its state. Thus each read operation can be tested to see if it works.

std::ifstream   data('mydata.txt');

int  value;
if (data >> value) {
    // The read worked.
}

std::string line;
while(std::getline(data, line)) {
    // Successfully read a line.
}

Each read operation should be checked to make sure it worked correctly.

int a,b;
if (data >> a >> b) {
    // Both a and b were read correctly.
}

You are not using operator>>. You get everything as a string then convert it. All the built in types have operator>> defined so you can simply read them (If the value read is not the correct type the stream is set bad and your code will automaically stop reading).

Also you can define operator>> fro user defined types so that you can use this simpler technique for nearly any object type (I provide an example below).

In modern C++ code you never see new/delete operations. That's because these return pointers and pointers have no ownership semantics. You should be using smart pointers so that ownership and thus lifetime are well defined.

std::unique_ptr  item(new Transistor(code,count,price,other));

Even better (if you have C++14/C++17)

std::unique_ptr  item  = std::make_unique(code,count,price,other);

How I would do it

Since operator>> for strings reads things that are space separated I would write a helper class for reading comma separated strings. Thus I can use read chaining to read all the things on a line in a single read operation.

struct CommaValueRef
{
    std::string& data;
    CommaValueRef(std::string& data) : data(data) {}
    friend std::istream& operator>>(std::istream& str, CommaValueRef& value) {return std::getline(str, value.data, ",");}
};

Since you are reading a file called "inventory.txt" I am going to assume that each line is a line of inventory. So I would define a class to hold the inventory and an operator>> that knows how to read a line of inventory.

class Inventory
{
    std::string type;
    std::string code;
    int         count;
    int         price;
    std::string other;

    friend std::istream& operator>>(std::istream& str, Inventory& value);
};

Now define the read like this:

std::istream& operator>>(std::istream& str, Inventory& value)
    {
        std::string line;
        if (std::getline(str, line))
        {
            Inventory         tmp;
            std::stringstream linestream(line);

            char c1, c2;
            linestream >> CommaValueRef(tmp.type)
                       >> CommaValueRef(tmp.code)
                       >> tmp.count >> c1
                       >> tmp.price >> c2
                       >> CommaValueRef(tmp.other);

            // A lot of developers would have combined the above
            // line and this if statement into a single expression
            // I separated them as I though it looked neater.
            if (linestream && c1 == ',' && c2 == ',')
            {
                // All the read's worked so put the data
                // into the actual value.
                value = std::move(tmp);
            }
            else
            {
                 // Something failed to read correctly.
                 // So force the stream into a bad state
                 // So the controlling loop can generate a message
                 // and exit.
                 str.setstate(std::ios::failbit);
            }
        }
        return str;
    }

Now reading the file looks like this:

Inventory   data;
while(inFile >> data)
{
    list.push_back(StockItemFactory::createItemFromInventory(data));
}

StackExchange Code Review Q#125156, answer score: 6