Custom template iterator on CSV reading class

Question

I made myself this code to read CSV files as I want to delve into machine learning. I do think that my code design is poor but I can't quite see why. The syntax of it does not feel right. I want to be able to iterate both over rows and columns but I'd rather not make two different kinds of iterators.

I'm sorry the code is a bit long to post on here but I'd really like a review on this.

What I hate the most is how I need to cast my iterators. I tried to avoid having to call a method to retrieve the value but I'm thinking this is probably one of the first things I would have to change :\

CSVFileReader.h

//==============================================================================
// CSVLine
// - Holds information on a specific row of a pre-loaded CSV file.
//==============================================================================

class CSVLine {
public:
    CSVLine();
    CSVLine(const CSVLine&);

    void Reset();
    void AddValue(std::string);

    unsigned int Size() const;

    const CSVLine& operator=(const CSVLine&);
    std::string& operator[](int);
private:
    std::vector<std::string> line;
};

//==============================================================================
// CSVIterator (TEMPLATE)
// - Can iterate on CSV classes (CSVData, CSVLine) if they provide
//   overloading for [] operator as well as return their size through Size()
//==============================================================================

template <class T_ITERABLE, class T_RETVAL>
class CSVIterator {
public:
    CSVIterator(T_ITERABLE& obj) : my_obj(obj), location(0) { /* blank */ }

    T_RETVAL GetFirst()             { return my_obj[0]; }
    T_RETVAL GetLast()              { return my_obj[my_obj.Size() - 1]; }
    T_RETVAL At(unsigned int index) { location = index;                 return my_obj[location]; }
    bool End()                      { return (location == my_obj.Size()); }
    bool Begin()                    { return (location == 0); }

    CSVIterator<T_ITERABLE, T_RETVAL>& obj) { out << obj; return out;}
    operator T_ITERABLE()           { return my_obj; }
    operator T_RETVAL ()            { return my_obj[location]; }
    T_RETVAL operator++(int)        { return my_obj[location++]; }
    T_RETVAL operator--(int)        { return my_obj[location--]; }
private:
    T_ITERABLE my_obj;
    int location;
};

class CSVData;  // Pre-declaration for typedefs

typedef CSVIterator<CSVData, CSVLine>       CSVRowIterator;
typedef CSVIterator<CSVLine, std::string>   CSVColIterator;

//==============================================================================
// CSVData
// - Holds the entire "array" of data. Allows data read-access to the user
//   through the CSVIterator class and its CSVRowIterator and CSVColIterator
//   pre-defined typedefs;
//==============================================================================

class CSVData {
public:
    void AddLine(CSVLine);
    unsigned int Size();

    CSVLine& operator[] (int);
private:
    std::vector<CSVLine> contents;
};

CSVFileReader.cpp

//=============================================
// CSVLine
//=============================================

CSVLine::CSVLine() { /* Blank */ }

CSVLine::CSVLine(const CSVLine& copy) {
    line.resize(copy.line.size());
    std::copy(copy.line.begin(), copy.line.end(), line.begin());
}

void CSVLine::Reset()                       { line.clear(); }
void CSVLine::AddValue(std::string value)   { line.push_back(value); }
unsigned int CSVLine::Size() const          { return line.size(); }

const CSVLine& CSVLine::operator=(const CSVLine& copy) {
    line.clear();
    line.resize(copy.line.size());
    std::copy(copy.line.begin(), copy.line.end(), line.begin());
    return *this;
}

std::string& CSVLine::operator[](int index) {
    if(index < line.size() && index >= 0 && line.size() > 0) { return line[index]; }
    else throw std::out_of_range("Error: Accessing CSVLine out of bounds");
}

//=============================================
// CSVData
//=============================================

void CSVData::AddLine(CSVLine line) { contents.push_back(line); }
unsigned int CSVData::Size()        { return contents.size(); }
CSVLine& CSVData::operator[] (int index) {
    if(index < contents.size() && index >= 0 && contents.size() > 0) { return (contents[index]); }
    else throw std::out_of_range("Error: Accessing CSVLine out of bounds");
}

//=============================================
// CSVFileReader
//============================================

CSVFileReader::~CSVFileReader() { /* ... */ }
void CSVFileReader::OpenFile(std::string file_name) { /* ... */ }
void CSVFileReader::LoadIntoMemory() { /* ... */ }

CSVData& CSVFileReader::GetData() { return file_data; }

This is an example of how I can manipulate a CSV file with it

int main(int argc, char** argv) {
    CSVReaderFactory csvrf;
    CSVFileReader* csvfr = csvrf.CreateFileReader();

    csvfr->OpenFile("C:\\Users\\Alexandre\\Documents\\test.csv");
    csvfr->LoadIntoMemory();

    CSVData& d = csvfr->GetData();

    std::cout << std::endl;
    CSVRowIterator row_iter(d);
    for(row_iter.GetFirst(); !row_iter.End(); row_iter++) {
        CSVColIterator col_iter((CSVLine)row_iter);
        for(col_iter.GetFirst(); !col_iter.End(); col_iter++) {
            std::cout << ((std::string)col_iter).c_str();
        }
        std::cout << std::endl;
    }

    csvrf.ReleaseFileReader(csvfr);

    std::cin.get();
    return 0;
}

Answer 1

Your design seems rather confused. You generally want iterators to be retrievable only from member functions of a class - the user shouldn't have to manually "create" an iterator. Secondly, you don't really need to create your own iterator types here. Your classes are fairly simple wrappers around std::vector, so just utilize the fact that it already provides iterators.

class CSVLine 
{
private:
    typedef std::vector<std::string>::iterator iterator;
    typedef std::vector<std::string>::const_iterator const_iterator;

public:
    /* .... */

    iterator begin()
    {
        return line.begin();
    }

    const_iterator begin() const
    {
        return line.begin();
    }

    iterator end()
    {
        return line.end();
    }

    const_iterator end() const
    {
        return line.end(); 
    }

private:
    std::vector<std::string> line;
};

And pretty much exactly the same thing with CSVData:

class CSVData 
{
private:
    typedef std::vector<CSVLine>::iterator iterator;
    typedef std::vector<CSVLine>::const_iterator const_iterator;
    //...
};

There's a few other things you should probably fix with your code as well. void AddValue(std::string); should take a const std::string&, so it doesn't end up copying the string twice (adding to a vector already performs a copy of the underlying data).

unsigned int Size() const; should be returning a typename std::vector<std::string>::size_type, which is usually a std::size_t. I'd simply add that as a typedef in your class:

typedef std::vector<std::string>::size_type size_type;
....
size_type Size() const;

std::string& operator[](int); should again be taking a size_type. You can't pass a negative parameter to [] for vector, so having it take a signed int makes no sense. You should also have const overloads of these methods:

const std::string& operator[](size_type i) const

Finally, it's hard to tell since there isn't any code for it, but CSVFileReader* csvfr = csvrf.CreateFileReader(); is never freed, at least not in main. This may or may not be a memory leak, depending on how it is declared and what csvrf.ReleaseFileReader(csvfr); does.

Answer 2

First I agree with @Yuushi in this situation you probably should not be writing your own iterator. The ones provided by the vector class should work fine.

But if you are in the situation to write your own iterator you need to do a few more things.

There a set of types that you should be defining (either inside the class or via traits)

    typedef std::input_iterator_tag     iterator_category;
    typedef XXXXXX                      value_type;
    typedef YYYYYY                      difference_type;
    typedef ZZZZZZ                      pointer;
    typedef AAAAAA                      reference;

Also you seem to be missing the fundamental requirements for an iterator:

    operator*()
    operator->()

And I don't think your iterator confirms to any of the standard iterator types. You want to look at that so people know how to use your iterators correctly.

See http://www.sgi.com/tech/stl/Iterators.html for details.

More information here: https://stackoverflow.com/a/1120224/14065