1
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Looks familiar. \$\endgroup\$ – Martin York Jun 2 '13 at 17:56
  • \$\begingroup\$ Don't use ALL CAPS Identifiers. Theses are reserved for macros and using them opens up your code to being stomped on by them. \$\endgroup\$ – Martin York Jun 2 '13 at 17:57
  • \$\begingroup\$ I don't know that this is true. Do you have any source on this bit of information ? \$\endgroup\$ – ApplePie Jun 2 '13 at 18:48
  • \$\begingroup\$ About the "Looks familiar", yes I got part of the idea on SO but I can't find the link to credit the author. I did not copy the code though, I only used it as an inspiration. It probably was better than what I did to be honest. \$\endgroup\$ – ApplePie Jun 2 '13 at 18:49
  • \$\begingroup\$ Are you questioning the bit about ALL CAPS. See any text book on C (usually chapter 1). This has been a convention from the beginning of C. The problem is that macros have no scope (and thus will trample on anything). Thus we manually scope them by defining macros so they can not overlap with other identifiers. The way we manually scope them is by reserving identifiers with ALL CAPS as macros. \$\endgroup\$ – Martin York Jun 3 '13 at 19:51
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That does feel better. Also, yes my design is confused, because I am confused ! I'll wait a bit to see if there are other reviews but your answer is a good candidate for check mark ! Thanks :) \$\endgroup\$ – ApplePie May 31 '13 at 3:00
  • \$\begingroup\$ You generally want iterators to be retrievable only from member functions of a class Don't agree. std::begin() and std::end() for eaxample. Or std::istream_iterator<> or std::ostream_iterator<> or any pointer type. \$\endgroup\$ – Martin York Jun 2 '13 at 18:00
2
\$\begingroup\$

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

| 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.