# Simple lightweight CSV reader in C++

My coding background is mostly from C and I've noticed that I tend to "use C++ as it was C" quite a lot. So this is a very small project I've written to improve my understanding of best practices in C++. I'm looking for a general review, with focus on:

• How well is it following standard C++ best practices?
• Is my use of std::ifstream considered RAII?
• General design overview, does it make sense (Variable names, structure of the class etc...)?

I also have two more specific design questions:

1. Should I move the functionality of Parse() to the constructor and just throw an exception if the file cannot be opened? Does it make sense to have the constructor open, read, and parse the file?
2. Is my use of GetRows() and CopyRows() methods clear for a potential user? Should I have exposed the underlying vector of vectors as public instead?

#pragma once
#include <string>
#include <vector>
#include <fstream>
#include <sstream>

using Delimiter = const char;
using Rows = std::vector<std::vector<std::string>>;

std::string m_FileName;
Delimiter m_Delimiter;
Rows m_Rows;

void ParseLine(const std::string &line) {
std::stringstream tokenizer(line);
std::vector<std::string> cols;
std::string col;

while (std::getline(tokenizer, col, m_Delimiter))
cols.push_back(col);

m_Rows.push_back(cols);
}

public:
: m_FileName(fileName)
, m_Delimiter(delimiter) {};

bool Parse() {
std::ifstream file(m_FileName);
if (!file.is_open())
return false;

std::string line;
while (std::getline(file, line))
ParseLine(line);

return true;
};

const Rows& GetRows() {
return m_Rows;
}

Rows CopyRows() {
return m_Rows;
}
};


Example usage (main.cpp)

#include "CsvReader.h"
#include "Logger.h"

int main() {
Logger(LogLevel::Critical) << "Could not open file!";
return -1;
}

Logger(LogLevel::Log) << "Example 1 (No copying)";
for (auto const &row : reader.GetRows()) {
std::stringstream output;
output << "[Row]: ";

for (auto const &col : row)
output << col << ", ";

Logger(LogLevel::Log) << output.str();
}

Logger(LogLevel::Log) << "Example 2 (Copy, possible to modifiy)";
for (auto &row : rows) {
for (auto &col : row) {
/* Do some modifications on the data... */
col.replace(0, 1, "");
col.replace(col.end() - 1, col.end(), "");
}
}

/* ... Do other things. Come back and view data. */
for (auto &row : rows) {
std::stringstream output;
output << "[Row]: ";

for (auto const &col : row)
output << col << ", ";

Logger(LogLevel::Log) << output.str();
}
}


I'll include the Logger.h as well for completeness, you can omit this in the review (Or don't, I don't mind the extra feedback but it's not really a part of my question and I guess it would fit better in a different post).

#pragma once
#include <iostream>
#include <sstream>

enum class LogLevel {
Log,
Warning,
Error,
Critical,
};

class Logger {
std::stringstream ss;
LogLevel logLevel;

public:
Logger(LogLevel l) : logLevel(l) {}
~Logger() {
switch (logLevel) {
case LogLevel::Log:
std::cout << "[LOG]: ";
break;

case LogLevel::Warning:
std::cout << "[WARNING]: ";
break;

case LogLevel::Error:
std::cout << "[ERROR]: ";
break;

case LogLevel::Critical:
std::cout << "[===CRITICAL===]: ";
break;
}

std::cout << ss.str() << std::endl;
}

template<typename T>
Logger& operator << (const T& arg) {
ss << arg;
return *this;
}

Logger& operator << (bool b) {
ss << (b? "true" : "false");
return *this;
}
};


1. Should I move the functionality of Parse() to the constructor and just throw an exception if the file cannot be opened? Does it make sense to have the constructor open, read, and parse the file?

Since it doesn't make sense not to call Parse(), I would say it is better to open the file and parse it in the constructor. That way, you also don't have to store the filename and the delimiter in the class anymore. But there might be other ways to deal with this, see below.

1. Is my use of GetRows() and CopyRows() methods clear for a potential user? Should I have exposed the underlying vector of vectors as public instead?

There is no need to have two different functions. GetRows() is enough, if the caller wants a copy, they can just write:

CsvReader reader(...);
...


This creates a new instance of Rows and copy-initializes it with the contents of the reference we got. Note that auto will never deduce a reference, so it will deduce CsvReader::Rows.

# Parsing vs. storing the results

I would separate the act of parsing from storing the results of parsing. Parsing is a one-shot action that reads a file and produces a vector of vector of strings, so it could just be a stand-alone function:

std::vector<std::vector<std::string>> ParseCSVFile(const std::string &filename, char delimiter) {
...
}


You could still declare a type alias for the result of ParseCSVFile if desired.

Alternatively, consider that you might not want to parse the whole file in one go, for example because it might be very large. The first part of the example usage you've shown just prints out every row. If you could make CsvReader a class that acts like a container that can be iterated over, then you could rewrite that example like so:

for (auto &row: CsvReader("assets/test.csv", ',')) {
Logger(LogLevel::Log) output;
output << "[Row]: ";

for (auto &col: row)
output << col << ", ";
}


# Make member functions that don't change member variables const

While GetRows() returns a const reference, the function itself is not marked const. You should mark the function as const as well, so the compiler can better optimize code:

const Rows& GetRows() const {
return m_Rows;
}


# Pass a std::istream& instead of a filename to the parser

By passing a filename to the parser, you've given the parser the responsibility to open the file. It also limits it to only open files. Consider instead passing it a reference to a std::istream:

std::vector<std::vector<std::string>> ParseCSVFile(std::istream &file, char delimiter) {
...
std::string line;
while (std::getline(file, line))
...
...
}


Now you can call it with std::cin as the argument for example, or have it parse data that is in a std::stringstream.

# Missing error checking

While in your original code you opened the file and checked whether opening was succesful, you never checked whether there was any error while reading the file. You don't have to do error checking for every I/O operation though; if you parse in the whole file at once with the while (std::getline(...)) loop, then only check after the while-loop that you actually reached the end of the input. If not, throw or otherwise report an error:

while (std::getline(file, line))
...

if (!file.eof())

Throwing exceptions has the benefit that either something must explicitly catch the error, or the program will abort with an error message, making it less likely that the error will be silently ignored. If you want to return a bool, then consider marking the function [[nodiscard]], or if you change your code to have a stand-alone function that returns a vector, then wrap it in a std::optional so there is a way to distinguish an empty file (which would produce an empty vector as output) from a read error.
• I don't see a reason why you could not just construct a new CsvReader at the point where you would call Parse(). Also, this has little to do with the size of the files; if you do store the whole parse result in a vector, then you will pay the price for that regardless of whether you parse in the constructor or in a separate member function. Sep 20 at 11:49
• This is not necessary; the way functions return large objects is already such that the caller will reserve memory for them and pass a pointer to the function so it knows where to store the return value. The temporary vector you have to create that you will return at the end of the function will also not result in a copy thanks to return value optimization that is mandatory for C++17, but most compilers already did anyway even for older versions of the C++ standard. Sep 20 at 12:08