- Classes aren't supposed to behave the way that "Level" does. In particular, the "LoadLevel" looks like it really wants to be a constructor, and the class should handle deallocation of the Map itself - i.e. in a destructor. Anything less is asking for hard-to-find bugs later on - put code in the right places (encapsulation).
- The Level also doesn't need to hold on to that ifstream object, so that should just be a local in the constructor.
- For constant values, use actual constants. Better yet, use an enum for a set of possible values that logically create their own "type" (like "direction"). #define hackery is no longer required in C++ and takes on unnecessary risk.
- Checking the ifstream fail() *after* trying to read the number of nodes is a bit of a problem.
- As mentioned, use a for loop rather than a while loop where it is called for.
- "using namespace std;" shouldn't have the :: in it. I'm surprised that compiles. Anyway, using it in *header* files is generally considered a bad idea.
//LevelLoad.h#ifndef LEVEL_LOAD_H#define LEVEL_LOAD_H#include <iostream>#include <fstream>#include <string>#include <vector>enum direction { NORTH, EAST, SOUTH, WEST, NUM_DIRECTIONS };class Node { // With proper design, we won't need these to be public. std::string BMPNames[4]; int adjacentNodes[4]; public: void Draw(direction d); Node() {} // To be stored in a std::vector (keep reading), Nodes need to be // default-constructible - thus we must explicitly define this ctor, since // there is another one provided. The default-constructed objects won't // really be useful, so we need to be careful to initialize every object // that's needed (see Level constructor, and comments about making this an // inner class). Node(std::istream& in);};class Level { // I will use the standard library class std::vector to hold the map. // It automatically tracks the length of data storage, and behaves basically // like a "resizable array" with minimal overhead - and also does memory // management for us. std::vector<Node> map; // A helper function for loading. void init(std::istream& in); public: // Note that we can't just do this with a temporary ifstream (like // "init(ifstream(filename))", because temporaries need to be kept constant, // and reading from a file stream changes that stream object. Level(const char *filename) { std::ifstream in(filename); init(in); } Level(std::istream& in) { init(in); }};#endif
//levelload.cpp#include "LevelLoad.h"using namespace std;void Node::Draw(direction d) { // do something with BMPNames[d] and adjacentNodes[d]}Node::Node(istream& in) { // In constructors, normally the best way to indicate a fatal error ("oops, // not possible to construct an object after all") is to throw an exception. // Here we want to do that if there's a problem with the input stream; we do // that by telling the input stream object to treat any problems as // exceptional, thus: in.exceptions(ifstream::eofbit | ifstream::failbit | ifstream::badbit); for (int i = 0; i < NUM_DIRECTIONS; i++) { in >> BMPNames >> adjacentNodes; // We can chain these actions the same way we would with printing to cout. // Note that this is treating the items as separated by *any* whitespace; // if you really want *line* at a time behaviour, investigate std::getline. }}// A helper function for loading. The user will not call this directly, but// instead just use a constructor. The user has the option to open the file// himself, or pass a string literal and let that constructor open the file.void Level::init(istream& in) { // As before, enforce stream validity. // For our purposes we only really need to do this here, but I've done it // in the Node class as well so that it can "stand on its own". // Another option would be to make the Node an inner class of Level, since // outside code probably shouldn't be mucking around with Nodes directly at // all. in.exceptions(ifstream::eofbit | ifstream::failbit | ifstream::badbit); // Count the nodes (in a local variable!) and resize the vector accordingly int nodeCount; in >> nodeCount; map.resize(nodeCount); // Read everything in for (int i = 0; i < nodeCount; i++) { map = Node(in); }}
//main.cpp#include "LevelLoad.h"int main() { Level test("test.txt"); // and it will clean itself up.}