Inserting an object pointer to a two-level map I'm working on requirement that involves creating a N x N table of blocks, with each block having a weight.

Consider the following classes


Block - The fundamental unit of the table, having a weight
RowBlock - A set of blocks that join together one row
BlockTable - A set of rows that join together to form the table


The program simulates inserting one block at a time to form the table altogether.

I know it is not a C++ thing to play around with pointers but rather use const references, but again this is part of a bigger problem that we are trying to solve.

This below code seems to work for now. But I'm a novice C++ programmer, want to know the potential pitfalls/vulnerabilities in the code

#include &lt;iostream&gt;
#include &lt;utility&gt;
#include &lt;stdint.h&gt;
#include &lt;cstdlib&gt;
#include &lt;map&gt;

class Block
{
    private:
        int m_value;
    public:
        Block(int value)
        {
            m_value = value;
        }
        void setvalue(int value);
        int getvalue();
};

class RowBlock
{
    private:
        std::map&lt;int, Block*&gt; m_rowBlock;

    public:
        RowBlock() = default;
        void insertBlock(Block* newBlock);
        const std::map&lt;int, Block*&gt; getList();
};

class BlockTable
{
    private:
        std::map&lt;int, RowBlock*&gt; m_BlockTable;
    public:
        BlockTable() = default;
        ~BlockTable();
        void insertBlockRow(Block* newBlock);
        const std::map&lt;int, RowBlock*&gt; getList();
};

int Block::getvalue(void)
{
    return(m_value);
}

void RowBlock::insertBlock(Block* newBlock)
{
    m_rowBlock.emplace(newBlock-&gt;getvalue(), newBlock);
}

void BlockTable::insertBlockRow(Block* newBlock)
{
    std::map &lt;int, RowBlock*&gt;::iterator iter;
    RowBlock *localRowBlock = NULL;
    iter = m_BlockTable.find( newBlock-&gt;getvalue() );

    if( iter == m_BlockTable.end() )
    {
        localRowBlock = new RowBlock;
        localRowBlock-&gt;insertBlock(newBlock);
        m_BlockTable.emplace(newBlock-&gt;getvalue(), localRowBlock);
    }
    else
    {

        localRowBlock = iter-&gt;second;
        localRowBlock-&gt;insertBlock(newBlock);
        m_BlockTable.emplace(newBlock-&gt;getvalue(), localRowBlock);
    }
}

const std::map&lt;int, RowBlock*&gt; BlockTable::getList()
{
    return m_BlockTable;
}

const std::map&lt;int, Block*&gt; RowBlock::getList()
{
    return m_rowBlock;
}

BlockTable::~BlockTable()
{
    std::map&lt;int, Block*&gt;::iterator iter_1;
    std::map&lt;int, RowBlock*&gt;::iterator iter_2;
    std::map &lt;int, RowBlock*&gt; tmpList_1 = getList();
    std::map &lt;int, Block*&gt; tmpList_2;

    for ( iter_2 = tmpList_1.begin(); iter_2 != tmpList_1.end(); iter_2++ )
    {
        tmpList_2 = iter_2-&gt;second-&gt;getList();
        for ( iter_1 = tmpList_2.begin(); iter_1 != tmpList_2.end(); iter_1++ )
            delete iter_1-&gt;second;
        delete iter_2-&gt;second;
    }
}

int main() {
    BlockTable newBlockTable;

    for (auto i=1; i&lt;=5; i++)
    {
        Block *newBlock = new Block(i*10);
        newBlockTable.insertBlockRow( newBlock );
    }

    std::map&lt;int, Block*&gt;::iterator iter_1;
    std::map&lt;int, RowBlock*&gt;::iterator iter_2;
    std::map &lt;int, RowBlock*&gt; tmpList_1 = newBlockTable.getList();
    std::map &lt;int, Block*&gt; tmpList_2;

    for ( iter_2 = tmpList_1.begin(); iter_2 != tmpList_1.end(); iter_2++ )
    {
        tmpList_2 = iter_2-&gt;second-&gt;getList();
        for ( iter_1 = tmpList_2.begin(); iter_1 != tmpList_2.end(); iter_1++ )
            printf("%d\n", iter_1-&gt;second-&gt;getvalue());
    }

    return 0;
}
General improvements

Names

C++ member and variable declarations don't tell you the type of an object; they tell you what operations on it are legal.  Prefer Block *newBlock (object dereferenceable to value of type Block) to Block* newBlock; the latter will only confuse you.  

Tradition is to use all lowercase and separate words with underscores, but your naming convention is fine too.  

Do you really mean "table"?

Technically, what you've written here is not a table, but closer to a jagged array.  You have no compile-time constraints forcing the members of each row to be of the same length, so you could have a table looking like this:


This image was pulled off the internet; the naming convention doesn't quite match your code.

Do you mean that to be the case?  

It's not just the varying row lengths that's strange.  Because each row is a map to the values of each block, the idea of each block having a fixed index is wrong.  In short, make sure your data structure matches your mental image of it.  

Finally, if you do want each block to have a fixed index and you want a rectangular table, I wouldn't bother with an intermediate data type RowBlock.  Instead, I would just have BlockTable manage the row/column distinction directly.  This also allows you to flatten your table, if performance demands: instead of n different rows of length n, you have a single array of length n*n.  What consumers of your class call the (r,c)th element is then internally at index n*r+c.  This reduces memory fragmentation.  I'm not going to flatten your tables for you, but it is something to consider.  

Memory management


  I know it is not a C++ thing to play around with pointers but rather use const references, but again this is part of a bigger problem that we are trying to solve.


No!  The important thing is to be aware of the lifetime of your memory and ensure that your code architecture manages it correctly.  If you need mutable references, or just to allow nullptr sentinels, then pointers are the way to go.  

Better still are the stdlib's smart pointers; choose std::unique_ptr or std::shared_ptr as appropriate.  

A usual way to handle this is to assume that the lifetime of each block and row should be the lifetime of the enclosing row and table, respectively.  If a consumer wants to preserve data beyond that time, they'll need to copy or swap it away.  

For example, you have lines like this: 

const std::map&lt;int, Block*&gt; getList();


Never return large objects (like std::map) by value, if you can avoid it.  

const std::map&lt;int, Block*&gt; &amp;getList() const;


is better (and const-correct).  

RAII

It should be hard to create your objects without initializing them.  (Ideally, it should be impossible, but for some of your objects,  induces undue performance limitations, so you'll have to trust your coworkers.)  This means you should support brace-initializion and swapping objects, so you don't need to create them twice.  For example, BlockTable should have lines looking something like this:

public:
    BlockTable() = default;
    BlockTable(std::initializer_list&lt;std::pair&lt;int,RowBlock&gt;&gt; &amp;&amp;elems) : 
        m_BlockTable(elems)
    {}
    template&lt;typename T&gt;
    BlockTable(T start, T end) :
        m_BlockTable(start, end)
    {}
    friend std::swap&lt;BlockTable&gt;(BlockTable &amp;&amp;left, BlockTable &amp;&amp;right);


If you manage memory properly, using smart pointers, etc., your destructors should be trivial.  The long and complex ones you've written are a code smell.  

Const-correctness

Your classes offer almost no support for const-correctness.  Make sure to mark procedures that do not mutate as such!  A consumer of your class who cares about const-correctness will have fits if you don't.  

ABI?

You may want variants of these classes for different sizes of integers, i.e. long long int, short int.  The easy way to do this uses templates, but then compile-time constraints can be hidden by typos, and you do not have a consistent binary layout across compilers.  I'm going to assume that you specialized to int on purpose; the templated version is not much more than a find-and-replace if done correctly.  

Synchronization

C++ is no longer a single-threaded language, but most code is still written that way.  I assume you externally synchronize any accesses to BlockTable objects in multithreaded contexts, in which case there is no problem.  

I/O

C-style I/O (i.e. printf) has been superseded by &lt;iostream&gt;.  Prefer that, but if you aren't taking user input or writing anything complicated out, it doesn't matter too much.  

MWE

You forgot to define Block::setvalue.  Did you mean

void Block::setvalue(int value)
{
    m_value=value;
}


perchance?  This also shows that your test suite (which appears to be main) is woefully incomplete.  Make sure the data structure behaves sanely with regard to inserted, non-random values! 

Other stylistic tips

auto is your friend.  Use it.  

The range-based for is your friend too.  Use that.  

If you have to manipulate iterators by hand, prefer ++iter.  iter++ requires the iterator to spend a lot of effort copying itself, only for the old version to get promptly thrown away.  

Specific rewriting

I don't understand the exact purpose of your data structure; the maps don't seem to do what your documentation in the question describes.  So I can't tell you how to make main more informative.  But even without understanding the most important semantics of your code, I can tell you that, if you can add a few constructors and automatic memory management to your data structures so that the following mechanical rewriting of main is correct, your code will be better.  

void main()
{
    BlockTable newBlockTable;
    for (auto const i : {1, 2, 3, 4, 5})
    {
        newBlockTable.insertBlockRow(Block(i * 10));
        //or better:
        //newBlockTable.insertBlockRow({i * 10});
        //or maybe even:
        //newBlockTable.insertBlockRow(i * 10);
    }
    //or best of all:
    //BlockTable const newBlockTable{10, 20, 30, 40, 50};

    //The names of your variables were too uninformative; I've changed them
    auto const &amp;row_map{newBlockTable.getList()}; //No need for complicated iterator types!  
    for (auto const &amp;key_and_row : row_map)
    {
        auto const &amp;block_map{key_and_row.second-&gt;getList()};
        for (auto const &amp;key_and_block : block_map)
            printf("%d\n", key_and_block.second-&gt;getvalue());
    }
    //If you aren't going to return anything but success, don't bother returning a value.  
}