C++ Simple 2D Array Wrapper

I know I should be using vectors, but I want to learn something.

Please review it and point out any mistakes. Thank you. Here it is:

template
class Table2D {
public:
    Table2D( unsigned int sizeX,
             unsigned int sizeY )
            : m_sizeX( sizeX )
            , m_sizeY( sizeY )
    {
        try {
            m_2DTable = new T[ m_sizeX * m_sizeY ]( );
        }
        catch( std::bad_alloc ) {
            std::cerr << "bad_alloc...\n";
        }
    }
    ~Table2D( ) { delete [ ] m_2DTable; }
private:
    unsigned int m_sizeX;
    unsigned int m_sizeY;
    T* m_2DTable;
public:
    void set( unsigned int x, unsigned int y, T val ) {
        m_2DTable[ x + ( y * m_sizeX ) ] = val;
    };
    T get( unsigned int x, unsigned int y ) {
        return m_2DTable[ x + ( y * m_sizeX ) ];
    };
};

Usage example:

Table2D ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
std::cout  f_2DTable( 3, 7 );
f_2DTable.set( 1, 4, 1.75 );
std::cout << f_2DTable.get( 1, 4 ) << std::endl;

Firstly, I assume you have something like #define __in up the top. This is sort of a Microsoft-ism, and really isn't needed in most of these parameter sets. When you're passing types that are neither pointers nor references, it's pretty obvious that they're input values. To me, it's simply extra clutter which makes the code harder to read and provides no benefit.

Secondly, this seems like an extremely memory heavy way of doing what is basically a map. You allocate space for m_SizeX * m_SizeY elements, but if the table is sparse, a majority of that space is going to go unused.

Thirdly, you should be passing and returning certain values by (const) &. For example, in your set method should probably be:

void set(unsigned int x, unsigned int y, const T& val)

And your get method should be:

T& get(unsigned int x, unsigned int y)

This should also have a const overload:

const T& get(unsigned int x, unsigned int y) const

Note that if you haven't initialized a value, it will happily return you whatever junk happened to be there when new allocated the memory. This is probably not what you want - you should likely have it return either a default value or error.

If I were to write this, I'd utilize a std::map and something like the following:

#include 
#include 

template 
class Table2D
{
private:
    std::map, T> table;

public:
    T& operator()(unsigned x, unsigned y)
    {
        std::pair p = std::make_pair(x, y);
        return table[p];
    }

    const T& operator()(unsigned x, unsigned y) const
    {
        std::pair p = std::make_pair(x, y);
        return table[p];
    }
};

This code assumes you want a default T() value (which will be 0 for all numeric types) to be inserted if it doesn't exist. Utilizing operator() returning a T& allows us to set and get values utilizing the same method:

Table2D ui_2DTable;
ui_2DTable(10, 20) = 50;
std::cout << ui_2DTable(10, 20) << "\n";

Further, if we do something like:

std::cout << f_2DTable.get(10, 10) << "\n";

We'll simply return a default value. Currently, this will return something from a random bit of memory, and causes undefined behaviour, which is bad.

Finally, your code can cause undefined behaviour if the user tries to utilize the copy constructor or copy assignment operator. That is:

Table2D ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
Table2D ui_construct(ui_2DTable);
Table2D ui_copy = ui_2DTable;

will have the pointer m_2DTable pointing to the same memory in all 3. If one goes out of scope, the memory will be deleted, while the others will still try to access it - if you're lucky, this will crash. I won't go into complete detail on how to fix this, but you can read about the so called "Rule of Three" here. Suffice to say, the version using std::map doesn't suffer from this problem (one of the nice benefits of standard containers is this comes "for free").

StackExchange Code Review Q#23941, answer score: 2