I'd probably be using [font=courier new,courier,monospace]std::vector[/font] or something like that, but yes, what you have is correct.*

That semicolon after the [font=courier new,courier,monospace]}[/font] at the end of the definition of the Map constructor is unnecessary. To move the constructor's code to the .cpp you just... move it just like you've got the destructor (i.e. put [font=courier new,courier,monospace]Map(int width, int height);[/font] in the header and the just copy 'n' paste the constructor into the .cpp (prefixing it with [font=courier new,courier,monospace]Map::[/font] of course)).

[size=2]*I say correct because there isn't anything immediately wrong with it, however it's not totally correct. The reason is that you allocate memory in the constructor several times. If one of the allocations succeeds, and then one fails after it and a std::bad_alloc exception is thrown, the allocated memory will be leaked because it will never be deleted (the destructor will not be called if the constructor fails and throws an exception). That's why it's nice to use something like std::vector that automatically and safely handles the ugliness of directly dealing with allocating and deallocating memory.

As Cornstalks said, it's correct.

I would just add that you can initialize everyting to zero with memset, rather than looping through everything:

[source lang="cpp"]for (int i = 0; i < MAP_HEIGHT; ++i){
tiles = new int[MAP_WIDTH];
memset(tiles, 0, sizeof(int) * MAP_WIDTH); // initialize each tile to 0
}[/source]

I would just add that you can initialize everyting to zero with memset, rather than looping through everything:

That's a good point, but it's the C way of doing things. C++ would use std::fill (or std::fill_n).

But again, if I was using [font=courier new,courier,monospace]std::vector[/font], I'd let the constructor do it for me.

You could have a vector of vectors:

class Map
{
public:
Map(int width, int height): width(width) {
std::vector<int> row(width, 0);
tiles.resize(height, row);
}

// Not required for current implementation.
// ~Map();
private:
int width;

// Can be derived from tiles.size()
// int height

// The space between the two angle brackets here might be required on older compilers.
std::vector<std::vector<int> > tiles;
};



It would be better to use a dedicated 2D storage class for this, unless you want to support jagged rows. You could use boost::multi_array, though I find it is unwieldy (maybe too general purpose), so I tend to provide my own simpler version as a wrapper around std::vector.

Something like the following:

template<class T>
class array2d
{
public:
array2d(unsigned w, unsigned h, const T &t = T())
:
storage(w * h, t),
width_(w)
{
}

unsigned width() const { return width_; }
unsigned height() const { return storage.size() / width_; }

T &operator()(unsigned x, unsigned y)
{
return storage[index(x,y)];
}

const T &operator()(unsigned x, unsigned y) const
{
return storage[index(x,y)]);
}

private:
std::vector<T> storage;
unsigned width_;

unsigned index(unsigned x, unsigned y) const
{
return (y * width_) + x;
}
};

The index() helper function uses the same maths the compiler does when you use a raw multi-dimensional array.

The map becomes:

class Map
{
public:
Map(int width, int height)
:
tiles(width, height, 0)
{
}

// Not required for current implementation.
// ~Map();
private:
// Can be derived from tiles.width()
// int width;

// Can be derived from tiles.height()
// int height

array_2d<int> tiles
};


Accessing x,y vs y,x is a an arbitrary convention. Just like row major and column major. The main thing is to choose complementary conventions, otherwise you'll incur lots of unnecessary cache misses. The processor likes sequential access.

So if you are storing the data as [i, j], then you should loop for(i ...) { for(j ...) { /* ... */ } }, regardless of which dimensions you are mapping onto i and j.