Best C++ way of doing things?

[EACR92yv]

Hi everyone!

I'm a C programmer that has been learning c++ for the past couple of days and I consistently encounter the same problem. Because of the interoperability with C code, I often find myself tempted to do things in a C way and to include C standard libraries, but this way I will not learn C++ correctly. I decided on doing a project from scratch (a terminal application that generates mazes for roguelikes) which is going to be written with only C++ functions and types. It turns out it is more difficult than I thought it would be and I wanted to ask for some help. My code feels clumsy and I feel like there's probably a much better way of doing things but I do not know it. I've included a short snippet which I feel is kind of spaghetti and I hope that someone here can tell me if something that I'm doing is not exactly in C++ best practices. The code reads files with possible room configurations. It's IO heavy and that's the area in which I'm worried I'm doing things wrong. Any comments on how to do this better in C++ will be greatly appreciated. Thanks!

#include <iostream>
#include <vector>
#include <string>
#include <fstream>
#include <sstream>

using namespace std;

class Room {
    public:
        Room();
        Room(int w, int h, vector<string> d) : width(w), height(h), data(d) {}
        int width;
        int height;
        vector<string> data;
};

vector<Room> getRoomLayouts(const string filename);

int main() {
    try {
        getRoomLayouts("rooms");
    } catch(const char* err) {
        cerr << err << endl;
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;
}

vector<Room> getRoomLayouts(const string filename) {
    ifstream file;
    file.open(filename);
    vector<Room> rooms;
    string currentline = "";
    if(file.is_open()) {
        getline(file, currentline);
        if(currentline != "START") throw "Error: Not a rooms file.";
        getline(file, currentline);
        while(currentline != "END") {
            if(currentline != "STARTROOM") throw "Error: Rooms file is not structured correctly.";
            getline(file, currentline);
            std::istringstream s(currentline);
            int w;
            int h;
            s >> w;
            getline(file, currentline);
            s.str(currentline);
            s >> h;
            vector<string> data;
            getline(file, currentline);
            while(currentline != "ENDROOM") {
                data.push_back(currentline);
                getline(file, currentline);
            }
            rooms.push_back(Room(w, h, data));
            getline(file, currentline);
        }
    }

    return rooms;
}

[jlb]

Well one thing you should be doing, in C or C++ is using meaningful variable and function names. Next in both modern C and C++ you should be declaring your variables closer to first use not in one big glob at the beginning of some scope.

Next, IMO, for this simple program you're making things more difficult by using exceptions, and you should learn to use class constructors, when possible, instead of calling accessory functions like istream.open().

My code feels clumsy and I feel like there's probably a much better way of doing things but I do not know it.

Much of this "clumsiness" is probably being caused by how your input file is organized, or how you're accessing the file. If you posted a small sample of your input file someone may be able to help you tweek the file format to make things easier.

[EACR92yv]

Thanks for the useful feedback!

[Manga]

Also not sure if you knew this but <sstream> already includes <string> so you don't need to #include both.

[icy1]

can make a custom exception class that derives from std::runtime_error or so. Not sure if it's the best way, but can have a static map of error codes (from an enum) to error descriptions. Then you can do something like

    if (!width || !height)
    {
        throw GameException(RoomDimensions);
    }

And a catch from main might look like

    catch (rogue::GameException& ex)
    {
        cout << "\nError "<< ex.error_code() << ": "<<
                ex.message() << endl;
    }

I added like 10 files to this repl project, hopefully showing okay practices ;D https://repl.it/@icy_1/SubtleAdmiredForce

[dhayden]

C++ I/O is less error prone that C I/O, but it's awkward and slow. The program below will parse your file. One big difference is that I didn't require width and height to be on separate lines, which makes reading them trivial. See my comments for other changes and advice.

#include <iostream>
#include <vector>
#include <string>
#include <fstream>
#include <limits>

// It's better to say your using exactly what you need rather than
// using namespace std
using std::vector;
using std::string;
using std::ifstream;
using std::numeric_limits;
using std::streamsize;

class Room {
public:
    Room() : width(0), height(0) {}

    // When passing read-only data that is big, pass by const reference
    // to avoid an expensive copy operation
    Room(int w, int h, const vector<string> &d) : width(w), height(h), data(d) {}
    int width;
    int height;
    vector<string> data;
};

// Warn the caller that you can throw an exception
vector<Room> getRoomLayouts(const string filename) throw (const char *);

int main() {
    try {
        getRoomLayouts("rooms");
    } catch(const char* err) {
	//Use '\n' instead of endl unless you positively MUST flush the
	// stream. Otherwise you can kill performance
        cerr << err << '\n';
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;
}

vector<Room> getRoomLayouts(const string filename) throw (const char *) {
    ifstream file;
    file.open(filename);
    vector<Room> rooms;
    string currentline = "";

    // No need to check if file is open since getline will fail in that case.
    if (!getline(file, currentline) || currentline != "START") {
        throw "Error: Not a rooms file.";
    }

    while (getline(file, currentline) && currentline != "END") {
	Room room;
	if(currentline != "STARTROOM") throw "Error: Rooms file is not structured correctly.";
	file >> room.width >> room.height;
	file.ignore(numeric_limits<streamsize>::max(), '\n'); // skip newline
	while (getline(file, currentline) && currentline != "ENDROOM") {
	    room.data.push_back(currentline);
	}
	rooms.push_back(room);
    }

    return rooms;
}

}

[TwilightSpectre]

dhayden wrote:

//Use '\n' instead of endl unless you positively MUST flush the
// stream. Otherwise you can kill performance
cerr << err << '\n';

Sort of true, but wrong at the same time. Yes, only use endl when you need to flush the stream. No, it won't kill performance; most console programs of any significance are not I/O bound performance-wise, particularly when you only have one output statement that needs to be flushed anyway; in addition, when you're printing an error, usually you'd care more that the output gets flushed than for performance.

In addition, the way you've written it you get no performance gains whatsoever. std::cerr is unbuffered, and tied to std::cout, which means it gets flushed every time you write to it.

Moreover, unless you explicitly specify otherwise the C++ streams are synced with the C output streams, which means even if you used the (nominally) buffered variant std::clog the streams are unbuffered regardless, so you still end up flushing whenever the C streams feel like it (which IIRC is at every newline).

Long-wided diatribe aside; this doesn't actually make a difference performance-wise. Your best hope for any performance gains would be to use std::cout or std::clog instead, but in almost all cases such performance gains would be nigh unnoticeable.

TL;DR: std::endl is fine.

dhayden wrote:

// When passing read-only data that is big, pass by const reference
// to avoid an expensive copy operation
Room(int w, int h, const vector<string> &d) : width(w), height(h), data(d) {}

... except when you proceed to immediately copy said data regardless, in which case (from C++11, at least) passing by value is either equally good or better. The OP's code is fine.

dhayden wrote:

// Warn the caller that you can throw an exception
vector<Room> getRoomLayouts(const string filename) throw (const char *);

https://stackoverflow.com/questions/88573/should-i-use-an-exception-specifier-in-c?noredirect=1&lq=1

[dhayden]

TL;DR: std::endl is fine.

I was hoping to prevent the OP from getting into the habit of always using endl. As you rightly point out, it's fine in this program, and it's fine with cerr, but I've seen way to many programs here that print every line to every stream using endl. The consequences can be severe.

Ditto with passing a vector by value vs. const reference: It's all about getting into good habits.

[TwilightSpectre]

My point was that in almost every case, std::endl is fine. Also, in cases like the OP's, where you are copying the value anyway, you should be passing by value to take advantage of move semantics.

[Enoizat]

TwilightSpectre wrote:

My point was that in almost every case, std::endl is fine.

There’s also who advises against its usage (not with std::cerr):
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rio-endl
Previous discussion:
https://github.com/isocpp/CppCoreGuidelines/issues/357