current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I wrote this code to read from a CSV text file (containing data such as 12,3,568,48,3,8 with no more than 3 digits to each number). It stores the numbers as char arrays in the vector values. However, this does seem like a clumsy way of doing this with resizing the vector and copying the chars. Is there a neater/more succinct way of doing this?

#include <iostream>
#include <fstream>
#include <assert.h>
#include <vector>

int main() {
    using namespace std;
    ifstream in;
    in.open("Data.txt");
    assert(in.is_open());
    vector<char*> values;
    const int MAXSIZE = 4;
    char thisVal[MAXSIZE];
    while(in.getline(thisVal,MAXSIZE,',')) {
        values.resize(values.size() + 1);
        values.back() = new char[MAXSIZE];
        strcpy(values.back(), thisVal);
    }
    in.close();
    for (char* cp: values) cout<<*cp<<endl;
    return 0;
}
share|improve this question

1 Answer 1

up vote 3 down vote accepted

Storing char* in a vector leads to leaks. instead use std::string, which will manage its own buffer and delete it as needed.

vector<string> values;

inserting to the back of a vector can be done with push_back or you can construct in place with emplace_back in C++11:

while(in.getline(thisVal,MAXSIZE,',')) {
    string str(thisVal);
    values.push_back(std::move(str));
    //std::move signals that I won't be using str anymore and that the underlying buffer can just be passed through to the new element. 
    // After this str will not be valid. You can still assign to it normally but until you do any result of calling a method on it will be implementation defined.
}

or

while(in.getline(thisVal,MAXSIZE,',')) {
    values.emplace_back(thisVal);
}

Failing to open a file is common enough to use an explicit if and print an error message instead of asserting (which won't do anything when compiled as a release version):

if(!in.is_open()){
    cerr << "failed to open file.";
    return 1; 
}

I return 1 here to signal to the OS that the program didn't finish successfully and a script using it can then react to it.

share|improve this answer
    
Thanks a lot, this is perfect :) –  Robin Hartland Feb 19 at 14:12

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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