patterncppMinor

File IO class in C++

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

fileclassstackoverflow

Problem

For fun, I've made a practical file IO class in C++ with the purpose of being light and quick, and extendable for custom parsers. Here is the code:

parser.h

//
//  parser.h
//
//  Created by Taylor Shuler on 4/8/15.
//
//

#ifndef _parser
#define _parser

#include 
#include 
#include 

#include 
#include 
#include 

using namespace std;

class Parser {
public:
    // constructors

    /// default
    Parser();

    /// copy
    Parser(const Parser &p);

    /// custom
    Parser(string filename);

    // destructor
    ~Parser();

    // public members
    string getFilename();
    list getContent();

    // operators
    Parser& operator=(const Parser &p);
    Parser& operator-=(const Parser &p);
    Parser& operator+=(const Parser &p);

    bool operator==(const Parser &p) const;
    bool operator!=(const Parser &p) const;

    bool operator(const Parser &p) const;
    bool operator=(const Parser &p) const;

    friend Parser operator+(const Parser &p1, const Parser &p2);
    friend Parser operator-(const Parser &p1, const Parser &p2);

    friend ostream& operator>(istream &instream, const Parser &p);
protected:
    // shared members
    void allocFilename(string filename, bool freeMemory = false);
private:
    // attributes
    char* filename;
    list content;
};

#endif /* defined(_parser) */

parser.cpp

```
//
// parser.cpp
//
// Created by Taylor Shuler on 4/8/15.
//
//

#include "parser.h"

void panic(string msg) {
cout Parser::getContent() {
return content;
}

Parser& Parser::operator=(const Parser &p) {
if (this == &p) {
return *this;
}

// set the new filename
allocFilename(p.filename);

// make room for the new content
content.clear();

// add in p's content
for (auto it = p.content.begin(); it != p.content.end(); it++) {
content.push_back(*it);
}

return *this;
}

Parser& Parser::operator-=(const Parser &p) {
for (auto it = p.content.begin(); it != p.content.end(); it++)

Solution

Let's start with the obvious:

-
using namespace std in the header file, very bad. If you've been looking around our site for a few months you should see this suggestion come up quite often, if not, then read up on the discussion and learn about it.

-
Again something that comes up a lot, a global identifier starting with underscore. In this case it is aggravated for being a macro and with a common name used by libraries:

#ifndef _parser
#define _parser

Read up about the reserved C++ identifiers. As a side recommendation, use ALL_UPPERCASE for #defines to differentiate them from actual C++ entities (the preprocessor is a mini-language inside C++).

-
Lots of unnecessary includes here:

#include 
#include 
#include

This is not a C program, but when you do need a C header, you should reference it using its C++ alias, that is, starting with a c and with no .h, e.g.: `. But you don't need any of those anyway if you are writing clean C++. You do seem to need (the C++ std::string header, not the same as

), which is missing.

- 
This was already mentioned in a OP comment:

Parser

 is a bad name for a class that simply splits a source file into lines. Parsing is the process of "analysing a string of symbols, either in natural language or in computer languages, conforming to the rules of a formal grammar". In simpler terms, parsing means making sense of a set of symbols and/or text. Your class is pretty far from that. Perhaps something like

TextFileReader

 would be a more honest name.

The less obvious things and some nitpicking:

- 
From a first glance, it looks strange that your class has so many operators for arithmetic and comparison. Since each "Parser" is just a container to the lines of a file, does it make sense to be able to add/subtract instances? Doesn't make much to me. Same for the comparisons. I can relate to comparing files for equality, but for

 ordering has never occurred to me. Not by contents, at least.

- 
Why do you use a

char* for the filename is beyond my understanding, specially when the method that clones it already takes its input as a std::string

. Storing it as a C++ string would be just so much simpler.

-

exit

ing a C++ program is about as crude as it gets when it comes to error handling. Please just throw an exception: 

void panic(const std::string & msg) {
    throw std::runtime_error("Error: " + msg + "\n");
}


The ideal would also be defining a custom exception type derived from

std::runtime_error

, if you want to refine it further.

- 
You can use range-based for with C++11 to further simplify code like this:

for (auto it = p.content.begin(); it != p.content.end(); it++)


Range-based:

for (const auto & line : p.content) { /* `line` is a string */ }


- 
Why do you have a

protected section in your class? No signs of inheritance around, so all the internal details should be private

.

-

getFilename() and getContent() should also be const

 since they don't (and shouldn't) mutate member data.

- 
A

std::list is probably going to have inferior performance if compared to a std::vector

 in this case due to poor data locality. With a list, each line is going to be allocated as a separate memory block (think about list nodes). Of course that without measuring it is impossible to say if this is a performance problem to your application, but at any rate, I would always start with a

vector` and only upgrade to other structures if there is a compelling reason.

Code Snippets

#ifndef _parser
#define _parser

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void panic(const std::string & msg) {
    throw std::runtime_error("Error: " + msg + "\n");
}

for (auto it = p.content.begin(); it != p.content.end(); it++)

for (const auto & line : p.content) { /* `line` is a string */ }

Context

StackExchange Code Review Q#86407, answer score: 4

Revisions (0)

No revisions yet.