1
\$\begingroup\$

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 <iostream>
#include <utility>
#include <stdint.h>
#include <cstdlib>
#include <map>

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

class RowBlock
{
    private:
        std::map<int, Block*> m_rowBlock;

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

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

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

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

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

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

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

const std::map<int, RowBlock*> BlockTable::getList()
{
    return m_BlockTable;
}

const std::map<int, Block*> RowBlock::getList()
{
    return m_rowBlock;
}

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

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

int main() {
    BlockTable newBlockTable;

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

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

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

    return 0;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ It would be great to show application of general solution on a specific problem. General solution usually implies coping with weird inputs and situations, not Foo's and Bar's. As it stands now, the post is off-topic due to lacking context. The code seems to use C style programming. There should be a way to flatten inner level or better aggregate keys and values, but unfortunately I'm not sure about relationship between inner and outer levels of the structure. Applying the design approach on a more realistic problem might help. \$\endgroup\$ Feb 20, 2019 at 18:40
  • \$\begingroup\$ that and renaming symbols into something at least remotely real would make it on-topic. I see that this is a genuine problem, but am failing to understand what it asks. There was a meta post about why Foo and Bar questions are off-topic, but I am failing to find the link. \$\endgroup\$ Feb 20, 2019 at 18:57
  • 1
    \$\begingroup\$ perhaps it was this meta answer, which includes a link to Why is hypothetical example code off-topic for Code Review? \$\endgroup\$ Feb 20, 2019 at 19:06
  • \$\begingroup\$ @Incomputable: I've made a reasonable edit. Do let me know if its answerable now. \$\endgroup\$
    – Inian
    Feb 20, 2019 at 19:49
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ: I've made a reasonable edit. Do let me know if its answerable now. \$\endgroup\$
    – Inian
    Feb 20, 2019 at 19:50

2 Answers 2

4
\$\begingroup\$

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: A jagged array named <code>jaggedArr</code>, pointing to three memory blocks label "0", "1", and "2", each themselves pointing to memory blocks labeled "1" and "2"; "3", "4", and "5"; and "6", "7", "8", and "9" (respectively).

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<int, Block*> getList();

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

const std::map<int, Block*> &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<std::pair<int,RowBlock>> &&elems) : 
        m_BlockTable(elems)
    {}
    template<typename T>
    BlockTable(T start, T end) :
        m_BlockTable(start, end)
    {}
    friend std::swap<BlockTable>(BlockTable &&left, BlockTable &&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 <iostream>. 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 &row_map{newBlockTable.getList()}; //No need for complicated iterator types!  
    for (auto const &key_and_row : row_map)
    {
        auto const &block_map{key_and_row.second->getList()};
        for (auto const &key_and_block : block_map)
            printf("%d\n", key_and_block.second->getvalue());
    }
    //If you aren't going to return anything but success, don't bother returning a value.  
}
\$\endgroup\$
0
1
\$\begingroup\$

Why is Block a class? It seems to have a single - effectively public - member, (which you described as weight but then gave the much less meaningful name of m_value), so it could simply be replaced by a plain int.

To use printf(), we need to include <cstdio> (and name it properly: std::printf()). Prefer C++ headers to C headers (<cstdint> rather than <stdint.h> - or just remove that include, since it's never used).

Prefer to use smart pointers and containers to manage ownership and simplify your memory management. When new and/or new[] are unavoidable, then make sure you exercise your tests under a memory checker such as Valgrind.

Prefer initialisers to assignment (compiler warnings should help you avoid uninitialized members):

    Block(int value)
      : m_value{value}
    {
    }

And everything that other reviewers have mentioned.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.