3
\$\begingroup\$

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,other);
            }
            list.tailAppend(t);
            //counter++;

        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

You've got properly formatted data, you say?

Why not use something like sscanf? Maybe sscanf exactly is not the best choice for this, but some sort of function that tries to parse the entire line in one go seems best. Then you instantly have all the variables you need.

Failing that, you could always just call getline multiple times by hand. It's better than using i here, because you're not sharing ANY code between iterations.

stringstream linestream(line);
string type;
string code;
string countRaw;
int count;
string priceRaw;
int price;
string other;
if(getline(linestream,type,',')
   && getline(linestream,code,',')
   && getline(linestream,countRaw,',')
   && getline(linestream,priceRaw,',')
   && getline(linestream,other,',')){

    count = atoi(countRaw.c_str());
    price = atoi(priceRaw.c_str());
    StockItem *t = factoryFunction(code, count, price, other, type);
    list.tailAppend(t);
}

If you use a factory method, you can take that big if out of the part where you read from input and move it to somewhere less crowded.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @Blarsssss I did just hide your print statements but that's because I think they're debugging, yes? \$\endgroup\$ – Pimgd Apr 8 '16 at 13:54
  • \$\begingroup\$ yeah I have a question if you have the time, I have some lines which only have four values so it doesn't include the other variable ( with this it completely skips the object ) how would I allow it to continue even with only four ? \$\endgroup\$ – Blarsssss Apr 8 '16 at 13:57
  • \$\begingroup\$ @Blarsssss parse the first four, then check if you can get a fifth; if not, type is other and other is empty string. I guess. This is why you normally put the type at the start and optional parameters at the end. \$\endgroup\$ – Pimgd Apr 8 '16 at 14:07
  • \$\begingroup\$ So I have just put getline(linestream,other,','); within the if statement rather than apart of it, seems to work @Pimgd \$\endgroup\$ – Blarsssss Apr 8 '16 at 14:20
  • 1
    \$\begingroup\$ The problem with atoi() is there is no way to detect a failure. So before use you need to validate that the string contains only valid input. \$\endgroup\$ – Martin York Apr 8 '16 at 16:39
6
\$\begingroup\$

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<StockItem>  item(new Transistor(code,count,price,other));

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

std::unique_ptr<StockItem>  item  = std::make_unique<Transistor>(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));
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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