2
\$\begingroup\$

I made a CSV class which manages CSV files. Is this good?, I'm beginner btw. It's probably bad but can you give me some tips!

Header file:

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

class CSVFile {
public:

    //Debug opening mode. Prints a debug message if successfully opened the file.
    static int debug;

    //Writes a new line after function
    static int addNewLine;

    //Does not write new line after function
    static int doNotAddLine;

    //Default Constructor
    CSVFile(const char* _name);

    //Create and open file with opening modes.
    CSVFile(const char* _name, int num);


    //Clear file, write a new row and end line
    //Opens file in write mode (fstream::out)
    void WriteRow(std::string txt, int num = 2);

    //Adds a new row to the file without clearing data and ends line
    //Opens file in append mode (fstream::app)
    void AppendRow(std::string txt, int num = 2);

    //Clears Data in the file
    void EmptyContents();

    //Returns value in cell. Returns 
    //last value if row or column is out of bounds.
    //Arguments: row and column of cell
    std::string GetCell(int row, int column);

    //Goes to next line by writing a newline character('\n')
    void NextLine();

    //Deletes file
    void DeleteFile();

    std::string operator()(int row, int column);

    //Overloading extraction operator for 2d vectors
    void operator>> (std::vector<std::vector<std::string>>& csvVector);

    //Overloading insertion operator for 2d vectors
    void operator<< (std::vector<std::vector<std::string>> myVector);

    //Parsing from text file to map. 
    // If first column of file is not a number, 
    // characters from a-z or A-Z gets converted to integers from 1-26
    void operator>> (std::map<int, std::string>& map);

    //Overloading insertion operator for map<int, string>
    void operator<< (std::map<int, std::string> map);
private:
    std::fstream file;
    std::string name;
};

Cpp file:

#include "CSVFile.h"

CSVFile::CSVFile(const char* _name) : name(_name) {
    file.open(name, std::fstream::app);
    file.close();
}

CSVFile::CSVFile(const char* _name, int num) : name(_name) {
    file.open(name, std::fstream::app);
    if (num == 1)
    {
        file << "Debug: File opened successfully\n";
    }
    file.close();
}

void CSVFile::WriteRow(std::string txt, int num) {
    file.open(name, std::fstream::out);
    file.clear();
    file.seekg(0);
    file << txt;
    if (num == 2) {
        file << "\n";
    }
    file.close();
}

void CSVFile::AppendRow(std::string txt, int num) {
    file.open(name, std::fstream::app);
    file.clear();
    file.seekg(0);
    file << txt;
    if (num == 2) {
        file << "\n";
    }
    file.close();
}

void CSVFile::EmptyContents() {
    file.open(name, std::fstream::out);
    file.close();
}

std::string CSVFile::GetCell(int row, int column) {
    file.open(name, std::fstream::in);
    file.clear();
    file.seekg(0);
    std::string rowData;
    std::string cellData;
    for (int i = 0; i < row; i++) {
        std::getline(file, rowData, '\n');

    }
    std::stringstream sso;
    sso << rowData;
    for (int c = 0; c < column; c++) {
        std::getline(sso, cellData, ',');
    }
    return cellData;
    file.close();
}

void CSVFile::NextLine() {
    file.open(name, std::fstream::app);
    file << "\n";
    file.close();
}

void CSVFile::DeleteFile() {
    remove(name.c_str());
}

std::string CSVFile::operator()(int row, int column)
{
    return this->GetCell(row, column);
}

void CSVFile::operator>> (std::vector<std::vector<std::string>>& csvVector) {

    std::vector<std::string> rows;
    file.open(name, std::fstream::in);
    file.clear();
    file.seekg(0);
    while (file.peek() != std::fstream::traits_type::eof()) {
        std::string row;
        std::getline(file, row, '\n');
        if (row != "")
        {
            std::stringstream string;
            string << row;
            std::string value;
            while (std::getline(string, value, ','))
            {
                rows.push_back(value);
            }
            csvVector.push_back(rows);
            rows.clear();
        }
        else
        {
            csvVector.push_back({ "" });
        }

    }
    file.close();
}

void CSVFile::operator<< (std::vector<std::vector<std::string>> myVector) {
    file.open(name, std::fstream::app);
    for (std::size_t r = 0; r < myVector.size(); r++)
    {
        for (std::size_t c = 0; c < myVector[r].size(); c++)
        {
            file << myVector[r][c];
            if (c != myVector[r].size() - 1) {
                file << ",";
            }
            else if (r != myVector.size() - 1) {
                file << "\n";
            }
        }
    }
    file.close();
}

void CSVFile::operator>>(std::map<int, std::string>& map)
{
    file.open(name, std::fstream::in);
    file.clear();
    file.seekg(0);
    while (file.peek() != std::fstream::traits_type::eof()) {
        std::string row;
        std::getline(file, row, '\n');
        if (row != "")
        {
            std::string keyString;
            std::string value;
            std::stringstream sso;
            sso << row;
            std::getline(sso, keyString, ',');
            std::getline(sso, value);
            if (keyString.find_first_not_of("0123456789") == std::string::npos) {
                map[std::stoi(keyString)] = value;
            }
            else {
                char keyChar = keyString[0];
                keyChar = std::tolower(keyChar);
                if (int(keyChar) > 96 && int(keyChar) <= 122) {
                    map[int(keyChar) - 96] = value;
                }
            }
        }
    }
    file.close();

}

void CSVFile::operator<<(std::map<int, std::string> map)
{
    file.open(name, std::fstream::app);
    for (auto pair : map) {
        file << pair.first << "," << pair.second;
        if (map.rbegin()->first != pair.first) {
            file << "\n";
        }
    }
    file.close();
}

int CSVFile::debug = 1;
int CSVFile::addNewLine = 2;
int CSVFile::doNotAddLine = 3;
\$\endgroup\$
4
\$\begingroup\$
//Default Constructor
CSVFile(const char* _name);

That's not a default constructor, by definition. This one-argument constructor ought to be explicit. Why does it take a C-style string for the argument? I'll probably formulate the file name via std::string or std::filesystem::path operations. If your implementation uses low-level C library of POSIX primitives so they take C strings, extract that from the C++ type inside the function.

Bottom line: use filesystem::path to pass file names.


void WriteRow(std::string txt, int num = 2);
Why does writing a row need to copy the txt argument locally? It is very odd to pass a string by value so it draws attention from reviewers even when it is a proper thing to do.

I'm guessing you really want: void WriteRow(std::string_view txt, int num=2)

And what's num do? For that matter, just what is "writing a row"? This is for CSV files, so I expect a list of values, not a "row". I'd better look at the implementation...

if (num == 2) {
        file << "\n";

say what? num' is a parameter that causes a newline to be automatically written after whatever was in the string, when num==2`. Are you kidding me?

//Clear file, write a new row and end line
well, that's not even right (see previous paragraph)


I see file is opened and closed with each call. So why does file need to be a member? It should be a local variable inside each function. It has no purpose as member data.


I have no idea how this is supposed to be used or how it works. There is a function NextLine that appends a newline?

GetCell opens the file, reads a line at a time until if finds the right line number, then returns only one column? Can you imagine how slow this will be to read a whole record, let alone a whole file full of records?

\$\endgroup\$
3
\$\begingroup\$

Use bool instead of int

Consider changing the static flags to bool:

static bool debug;
static bool addNewLine;
static bool doNotAddLine;

Having both addNewLine and doNotAddLine does not make sense. Actually, since you are not using any of these flags you can simply remove them.

Multiple constructors

Instead of having multiple constructors, you could have created one with a default value:

CSVFile(const char* _name, bool debug = false)
{
    file.open(name, std::fstream::app);
    if (debug)
    {
        file << "Debug: File opened successfully\n";
    }
    file.close();
}

Passing objects by reference

Every time you pass a std::string by value you are unnecessarily making a copy of the string. Consider changing:

void WriteRow(std::string txt, int num = 2);

To the following:

void WriteRow(const std::string& txt, int num = 2);

I'm not sure what is going on with num = 2. It would be much more clear to use:

void WriteRow(const std::string& txt, bool isNumber = true);
\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much for all the tips, I'll add them now itself. I know that those static flags looked weird and useless haha, I was trying to learn them so I experimented with them lol. \$\endgroup\$
    – ihsan
    Oct 24 '21 at 13:53
  • 1
    \$\begingroup\$ @ihsan, cool! Note that you should not edit your question as this will invalidate the responses. Additionally, I would advise you to not accept an answer so quickly. This may discourage people from adding additional answers that may have given you more insights =) \$\endgroup\$
    – jdt
    Oct 24 '21 at 14:00
  • \$\begingroup\$ @upkadjt sorry my bad! im pretty new to code review \$\endgroup\$
    – ihsan
    Oct 24 '21 at 14:02
1
\$\begingroup\$

This code is nicely presented, which is great for reviewing. It would have been nice to see some usage examples, or (even better) some unit tests. One of the things that makes it hard to write good unit tests is that we've chosen to represent a file; if we make our core code work with std::istream and std::iostream, then it will be more testable, and we can still provide wrappers that can open files to obtain streams.


I did notice¹ in GetCell() this unreachable code:

return cellData;
file.close();

That last line will never be reached. It's actually not a problem here, since the std::file destructor will close for us anyway. I recommend only calling close() when we care about the result (e.g. when we write to the file).


#pragma once is non-standard. Prefer to use conventional #ifndef/#define include guards for portable headers.


These three members seem never to be used:

//Debug opening mode. Prints a debug message if successfully opened the file.
static int debug;

//Writes a new line after function
static int addNewLine;

//Does not write new line after function
static int doNotAddLine;

This constructor looks a little strange:

CSVFile::CSVFile(const char* _name, int num) : name(_name) {
    file.open(name, std::fstream::app);
    if (num == 1)
    {
        file << "Debug: File opened successfully\n";
    }
    file.close();
}

Firstly, num is pretty meaningless as an argument name. Is it the number of lines we're going to read? No, it seems that it's a flag to control whether we write a debug message to the end of the data file - that looks very wrong. Normally, debug messages should go to the std::clog stream.


It certainly seems odd that almost every operation needs to open and close the file. I would expect such a class to read once into a matrix (e.g. array of arrays) when constructed or explicitly asked to read, and only write when it is asked to.


This character-handling code looks problematic in several ways:

            char keyChar = keyString[0];
            keyChar = std::tolower(keyChar);
            if (int(keyChar) > 96 && int(keyChar) <= 122) {
                map[int(keyChar) - 96] = value;
            }

We don't know whether keyString is empty, which could make keyString[0] an invalid access.

std::tolower() has a problematic interface that in practice requires us to convert the argument to unsigned char before calling (because char may be signed, and we're not permitted to pass negative values other than EOF).

Finally, comparing characters against specific numeric values isn't portable, as we don't know which character coding is in use. I'm guessing you're working on an ASCII (or similar, e.g. ISO 8859.1) platform, where those numbers represent '`' and ~ respectively. To make this portable, we need to do a string search like we did when we checked whether keystring was composed of digits, but this time testing whether the character is in "abcdefghijklmnopqrstuvwxyz{|}~".


We're lacking good handling of errors throughout the program. Consider which operations can fail:

  • std::fstream::open()
  • std::getline() And some can throw exceptions:
  • std::stoi()

(That's not necessarily a complete list, just some functions I spotted during a skim read).

Make sure your program knows what to do when (for example) the file doesn't exist, or isn't readable by the user, or isn't writable. Or if there's a numeric value too big to represent (and have we handled negative values properly?).

To start with, I wouldn't even attempt to support numberic values in the files - treat everything as a string, at least until you have that robust enough to move on.


¹ More accurately, my compiler warned me about this - and so should yours, so check that you've enabled a full set of warnings. If it helps, I used g++ -std=c++20 -fanalyzer -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion -Weffc++ -Wuseless-cast.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for all the help. I don't know anything about the warnings and stuff you said in the last paragraph. Also, can you explain to me whether it is possible that I can give a warning to the user if there are some errors like the ones you said? \$\endgroup\$
    – ihsan
    Oct 24 '21 at 14:07
  • \$\begingroup\$ Yes, you can write diagnostics to the standard error stream std::cerr. Although, as this is a library, it's usually better to find some way to inform the calling program (via an exception, or through the return value), and let that decide how (and whether) to report that to the user. In some cases, there's no need for a message; for example if we want to open a file in one location, but fall back to a different file if that one isn't present. \$\endgroup\$ Oct 24 '21 at 14:28
  • \$\begingroup\$ How to enable warnings depends completely on which compiler you're using, which is why I didn't go into detail. But do be aware that most compilers only report errors which are required by the standard unless you ask for more warnings to be reported, so we nearly always need to find how to do that. \$\endgroup\$ Oct 24 '21 at 14:31

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.