Does my code look nice?

[danielmccarthy]

Hey,
Just curious if I have coded this nicely. Its a small snippet of some code that works with my inventory in my game. Basically the inventory is just a holder of a ItemWindow. This makes sense to me because you don't want to have to write separate code for the shops and the bank ect ect.

void ItemWindow::addItem(u16 itemId, u32 quantity)
{
    s16 numOfItems = 0;
    if (!ItemHandler::isItemStackable(itemId))
    {
        numOfItems = quantity;
        quantity = 1;
    }
    do
    {
        u16 curItemX = this->curItemSlot * 40;
        u16 curItemY = this->curItemRow * 40;
        ItemSlot* iSlot = new ItemSlot(itemWnd);
        iSlot->createItem(itemId, quantity, curItemX, curItemY);
        itemSlot.push_back(iSlot);
        if (this->curItemSlot == this->itemsPerRow-1)
        {
            this->curItemRow++;
            this->curItemSlot = 0;
        }
        else
            this->curItemSlot++;

        numOfItems--;
    } while(numOfItems > 0);
}

[Duthomhas]

Your code is gorgeous, easy-to-read, understandable. Would that all coders wrote so well.

[ne555]

> ItemSlot* iSlot = new ItemSlot(itemWnd);
http://www.cplusplus.com/forum/general/138037/#msg731921

[mutexe]

you could prepend 'm_' or just '_' to your member variables and then remove all of the 'this->'. that way you could instantly see that your dealing with member variables.

void ItemWindow::addItem(u16 itemId, u32 quantity)
{
    s16 numOfItems = 0;
    if (!ItemHandler::isItemStackable(itemId))
    {
        numOfItems = quantity;
        quantity = 1;
    }
    do
    {
        u16 curItemX = m_curItemSlot * 40;
        u16 curItemY = m_curItemRow * 40;
        ItemSlot* iSlot = new ItemSlot(itemWnd);
        iSlot->createItem(itemId, quantity, curItemX, curItemY);
        itemSlot.push_back(iSlot);
        if (m_curItemSlot == m_itemsPerRow-1)
        {
            m_curItemRow++;
            m_curItemSlot = 0;
        }
        else
            m_curItemSlot++;

        numOfItems--;
    } while(numOfItems > 0);
}

[Duthomhas]

@ne555
He is allocating and storing a new item. That seems me appropriate, and not what you complained about in the link. (Or do I misread?)

@mutexe
I personally think that m_ stuff is uglier and harder to parse. In the original code, the this-> isn't even really necessary, as you show by use in your patched example.

Personally, I think that "I'm a member"-style quantifiers should stand out somewhat to make them less visually crowded with the identifier, which this-> does without mangling the identifier.

That's my opinion, though.

[SamuelAdams]

Formatting looks great, now if you just added a few comments.

[Lowest0ne]

I prefer the m_ because it means that I can have member functions with the same name:

int Person::age( void ) const { return m_age; }

I see many different concerns in this function. In fact, I end up with 9 functions if I extract them all.

For example:
line 16 through 22 into: updateSlot()
line 17 becomes: endOfSlot()
line 18-19 become: nextRow()
line 22 becomes: nextSlot()

bool ItemWindow::endOfSlot( void ) const
{
  return this->curItemSlot == this->itemsPerRow -1;
}

void ItemWindow::nextRow( void )
{
  this->curItemRow++;
  this->curItemSlot = 0;
}

void ItemWindow::nextSlot( void )
{
  this->curItemSlot++;
}

void ItemWindow::updateSlot( void )
{
  this->endOfSlot() ? this->nextSlot() : this->nextRow();
}

Now it is easier to see that updateSlot needs to make sure that it doesn't go out of range, and it won't muck up the original function when you do it.

[danielmccarthy]

Thanks for everyone's responses :)

[ne555]

> He is allocating and storing a new item
In that case I would want to see the type of `itemSlot'
container<ItemSlot*> is ugly and would be error-prone as discussed (the memory management would not be in `addItem()' but somewhere else)
container< smart_ptr<ItemSlot> > would be innefficient


It would be necessary if it is planning on using polymorphism on the elements

[Duthomhas]

As I really don't know much more about his memory management than what he has posted, I can't volunteer any real suggestion about design issues.

[danielmccarthy]

All itemSlot memory is freed in the itemWindow's destructor.

[danielmccarthy]

ItemWindow::~ItemWindow()
{
    itemWnd->remove();
    for (int i = 0; i < this->itemSlot.size(); i++)
    {
        delete this->itemSlot[i];
    }
}

[ne555]

And that's error prone.
You also coded a proper copy constructor and assignment operator, ¿right?

¿is there a good reason that you can simply use container<ItemSlot>?

         ItemSlot* iSlot = new ItemSlot(itemWnd);
        iSlot->createItem(itemId, quantity, curItemX, curItemY);

¿why is `createItem()' separated from the constructor?

[danielmccarthy]

createItem is seperated from the constructor because its neater. It says it in black and white createItem. Anyone looking at this code knows exactly what is going on. If I allowed the item to be created in the constructor it would be confusing. I am also using a vector because the ItemWindows number of item slots needs to be set at runtime.

[ne555]

I didn't complain about the use of std::vector
I'm complaining that you are storing (raw) pointers


> It says it in black and white createItem
¿what's that?

> createItem is seperated from the constructor because its neater
¿is it valid, and desirable, to not call createItem and operate on an ItemSlot?

[JLBorges]

> All itemSlot memory is freed in the itemWindow's destructor.

// ...
ItemSlot* iSlot = new ItemSlot(itemWnd);
iSlot->createItem(itemId, quantity, curItemX, curItemY);
itemSlot.push_back(iSlot); // *** what happens if this throws? ***
// ... 

If it is possible to use value semantics, use value semantics.
If not, encapsulate resource management; use RAII (use a smart pointer)

One of the smallest additions is actually great in its impact. It's make_unique:

auto u = make_unique<some_type>( constructor, parameters, here );

The reason make_unique has important impact is that now we can teach C++ developers to mostly never use explicit new again. ...

With draft C++14, we can say simply: Don't use owning raw pointers, new, and delete, except rarely when implementing low-level data structures. ...

- Herb Sutter, https://isocpp.org/blog/2013/04/trip-report-iso-c-spring-2013-meeting

[danielmccarthy]

How do the unique pointers work? I heared those type of pointers have counters on them to predict when they need to free the object

[JLBorges]

std::uniqe_ptr<> implements strict single ownership semantics; it is slim and it is fast.
http://www.drdobbs.com/cpp/c11-uniqueptr/240002708

std::shared_ptr<> implements shared ownership semantics; it is a reference counting pointer.
http://msdn.microsoft.com/en-us/library/hh279669.aspx

[LB]

Where do the magic number 40s come from?

[danielmccarthy]

the 40s are the width and heights. I changed them to enums moments after writing this post