Basic CSV Parser in C++

Question

This was written down solely as a mean to practice basic C++ and isn't meant to serve any production purposes. Clearly, the implementation can't handle all input formats: it always expects the CSV to have a header line, it doesn't treat quoted commas well, the file has to have a newline in the end, hardcoded separator, etc.

I'm mostly interested in what am I doing wrong from the effectiveness and idiomatic perspective. I'm sure there's something wrong even with these 60 lines. For instance, I don't like the mech of filling data, I feel there's too much tossing stuff around.

Do I feel right about it? Can it be done in a more concise and effective way? Anything else, maybe like a forgotten const, or a totally non-idiomatic way to do something?

In short, how can I make this code (and subsequently, my C++) better? Thank you.

#include <iostream>
#include <vector>
#include <map>
#include <string>
#include <fstream>
#include <sstream>
#include <stdexcept>

template <typename OutputIterator>
void explode(const std::string& input, char sep, OutputIterator output) {
    std::istringstream buffer(input);
    std::string temp;
    while (std::getline(buffer, temp, sep)) {
        *output++ = temp;
    }
}

class CSV {
private:
    std::vector<std::string> headers;
    std::vector<std::map<std::string, std::string>> data;
    size_t length{};
public:
    explicit CSV(const std::string& filename) {


        std::ifstream ifs(filename);
        std::string s;

        std::getline(ifs, s, '\n');
        explode(s, ',', std::back_insert_iterator<std::vector<std::string>>(headers));

        std::map<std::string, std::string> temp_map;
        std::vector<std::string> temp_values;
        while (std::getline(ifs, s, '\n')) {
            explode(s, ',', std::back_insert_iterator<std::vector<std::string>>(temp_values));
            for (size_t i = 0, headers_size = headers.size(); i < headers_size; ++i) {
                temp_map[headers[i]] = temp_values[i];
            }
            data.emplace_back(temp_map);
            temp_values.clear();
            temp_map.clear();
            ++length;
        }
    };

    const std::map<std::string, std::string>& operator[](const size_t& index) {
        if (index < 0 || index >= length) throw std::runtime_error("Invalid index");
        return data[index];
    }
};

std::ostream& operator<<(std::ostream& os, const std::map<std::string, std::string>& m) {
    for (const auto& el: m) {
        os << el.first << ": " << el.second << ", ";
    }
    return os;
}

int main() {
    CSV c("data.csv");
    std::cout << c[0] << std::endl;
    return 0;
}

Answer 1

This is something I have done several times over the last 20 years, generally in a minimal fashion just for test input and single-use utilities, so like yours it doesn't handle bells and whistles but is simple and fast.

Just reading through your code, it needs comments. I see what explode does: split an input at a delimiter and send the pieces to an output iterator.

The main CSV class though, the first thing I wonder is why you have a map for data? I don't think of CSV as a key/value type of format, so what's it doing?

OK, it reads "headers" as one line only, so it's not like HTTP headers. Later, I see it's supposed to be field names, also in the same CSV.

So, after splitting a line, you do:

for (size_t i = 0, headers_size = headers.size(); i < headers_size; ++i)
    temp_map[headers[i]] = temp_values[i];

So, you are storing the fields by name, as given in the header.

Then a vector of these maps holds all the lines.

---

That is a rather inefficient and memory hungry way to do it. All the lines have the same named fields. Maps are dynamic data structures, and thus slow due to memory allocation and cache misses when using them.

You could store all the fields in a vector or other linear collection, and keep the names separately. Then, have access functions that can look up by name by finding the name in the headers first, and using the resulting index number to look up the field in the vector.

---

data.emplace_back(temp_map);
temp_map.clear();

You should use std::move since you are done with the temp_map anyway. This will prevent the expensive re-copying of the entire map, which is pointless since you just throw away the original. Or, you could start by pushing an empty element and get a reference to that, to use directly instead of a temp_map.

You don't need length as it tracks the size of the vector. Just use the vector size directly to determine if an element is in range or not.

const std::map<std::string, std::string>& operator[](const size_t& index) {

Why are you passing index by reference? It's a simple machine-word integer type, so should be passed by value.

This member function should also be declared as const as it accesses the data in the class but does not modify it.

so how do I do it?

Your explode function is fairly general, and could be used for splitting a line of text anywhere that is needed, not just as part of this CSV reader. For example, splitting the fields out of a date, splitting on a '-' or '/' character.

I call this split and it is modeled after Perl's function of the same name. There is also a similar Boost library function, also called split.

In my most recent version, I wanted to avoid memory allocation of a vector of results. I do that by returning a std::array instead, with the size known at compile time. This means that the memory is stored by the caller as a simple stack-based variable and no dynamic memory allocation is needed.

Rather than copy every single string, I return string_view which points back to the original input. It does not need to copy the contents nor allocate memory. Note that it does mean that the result array has a lifetime limited to that of the input string, unlike yours which collects all the lines before returning.

template <size_t N>
auto split (char separator, std::string_view input)
{
    std::array<std::string_view, N> results; 
    auto current = input.begin();
    const auto End = input.end();
    for (auto& part : results)
    {
        if (current == End) {
            const bool is_last_part = &part == &(results.back());
            if (!is_last_part)  throw std::invalid_argument ("not enough parts to split");
        }
        auto delim = std::find (current, End, separator);
        part = { &*current, size_t(delim-current) };
        current = delim;
        if (delim != End) ++current;
    }
    if (current != End)  throw std::invalid_argument ("too many parts to split");
    return results;
}

And that's another difference: I don't parse the entire file before returning, but parse each line as I consume it. There's no need to store the parsed results for everything, unless you are going back and forth through the data before storing it away in its final form.