4
\$\begingroup\$

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;
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you so much for your answer. I started to kind of refactor this, and came to realize: what if, while exploding the string, we do it with a simple class that takes the input string and then returns std::string_views of its parts in succession? Right now I've implemented it with custom .has_next() and .next() methods, but it could be an iterator as well. Would that make sense? That way we wouldn't even copy the input string to begin with, so the additional memory usage would be kept to minimum. And yeah, I got rid of different maps over and over. \$\endgroup\$ – Pavel Gurkov Jun 3 at 19:21
  • 1
    \$\begingroup\$ Yes, make it an iterator (see Boost Iterator Facade) and the object supports the range semantics so you can feed it directly to a range-based for loop. e.g. for (auto sv : split(s,delim)) Actually you might want to present the label as well, since your files individually define the order (and presence) of the named fields. \$\endgroup\$ – JDługosz Jun 4 at 14:44
  • 1
    \$\begingroup\$ @PavelGurkov In my current work, I drop the array of string views directly into a struct that is defined to represent that, converting each string to the proper actual type such as int or date. (The only code I have to write for a specific struct is to make sure the input fields are in the right order as expected by aggregate initialization) \$\endgroup\$ – JDługosz Jun 4 at 14:49

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.