2
\$\begingroup\$

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?

CsvReader.h

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

class CsvReader {
    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:
        CsvReader(const std::string& fileName, Delimiter delimiter)
            : 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() {
    CsvReader reader("assets/test.csv", ',');
    if (!reader.Parse()) {
        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)";
    std::vector<std::vector<std::string>> rows = reader.CopyRows();
    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;
        }
};
\$\endgroup\$
1
1
\$\begingroup\$

Answers to your 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?

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(...);
...
auto copy_of_rows = reader.GetRows();

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())
    throw std::runtime_error("Error while reading file");

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.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for the feedback! I will have to play around a bit with the different suggestion to get a better understanding of them and how they change the usage before i can decide which implementation to go for. Now this was just a made up project with no clear goal in mind, but it will certainly help me make better plans for future real-life scenarios. A question though; I deliberately seperated parsing from the constructor since i had large files in mind, thereby giving the user control of when to initiate the parsing. With that in mind, is what i did an acceptable and/or good approach? \$\endgroup\$
    – Smurker
    Sep 20 at 11:47
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – G. Sliepen
    Sep 20 at 11:49
  • \$\begingroup\$ Good point. Another follow up question; If i take the route of not storing any data and simply just return it, would that not lead to unnecessary copies being made of the 'vector of vectors of strings'? I don't know if this is something to worry about though, but could a potential solution be to pass in a target container in the function call? \$\endgroup\$
    – Smurker
    Sep 20 at 11:56
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – G. Sliepen
    Sep 20 at 12:08
  • 2
    \$\begingroup\$ @G.Sliepen even without NRVO/RVO the vector would be moved on output - not copied. In this case the difference between moving and NRVO is negligible considering everything else that's being done. \$\endgroup\$
    – ALX23z
    Sep 20 at 17:02

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.