HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Box-stacking problem in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
problemboxstacking

Problem

I have tried to write code of Box stacking problem (mentioned here) in C++ .
Kindly give me some views on what mistakes I might have made and how I can improve.
It is running for the two inputs I have provided.

```
//============================================================================
// Name : Boxstacking.cpp
// Author :
// Version :
// Copyright : Your copyright notice
// Description : Hello World in C, Ansi-style
//============================================================================

#include
#include
#include
#include
#include

using namespace std;
class dimension;
typedef shared_ptr > PVecDim;
typedef vector VecDim;
typedef vector::iterator VecDimIter;
typedef shared_ptr > PVecInt;
typedef vector VecInt;
typedef vector::iterator VecIntIter;
typedef shared_ptr > PDim;
typedef vector Dim;
typedef vector::iterator DimIter;

struct dimension {
int height, width, length;
dimension(int h, int w, int l) : height(h), width(w), length(l) {

}
dimension() : dimension(0,0,0) {

}
};

class BaseComparator {
public:
bool operator() (const dimension& a, const dimension& b) {
if(a.widtha.length > b.widthb.length)
return true;
else
return false;
}
};

class BoxStacking {
public:
int doBoxStacking(PVecDim inList) {

int i,j;

VecDimIter it;
coutbegin(); it != inList->end(); ++it) {
coutheightlengthwidthbegin(), modList->end(), compare) ;
coutbegin();dit!=modList->end();++dit) {
coutreserve(modList->size());
M->resize(modList->size());
vector prev(modList->size(), -1);

(M)[0] = (modList)[0].height;

for(i=1;isize();++i) {
(M)[i] = (modList)[i].height;
int max = (*M)[i];
for(j=i-1;j>=0;--j) {

if((((*modList)[i].length begin(); it!=bs.mOutList->end(); ++it) {
outbegin(); it!=inList->end(); ++it) {

Solution

What quickly jumps out:

typedef vector VecDim;
        //              ^     Pointer
typedef vector::iterator VecDimIter;
        //              ^     Pointer


Its very rare to see "raw" pointers in good C++ code. This is because now you have to do memory management on the code. Pointers are useful for implementing the low down dirty bit of a structure and are used in the bowls of classes to implement higher order structures (because you can use constructor/destructor to guarantee they don't leak). But up in user space you should be using smart pointers.

But in reality I don't see any need even for smart pointers here. Just declare them as objects.

typedef vector VecDim;
typedef vector::iterator VecDimIter;


Now you no longer have any memory leaks (its not as if dimension is a huge object the standard copy constructor will work (and 99% of the time it will be elided)).

Also I would declare VecDemIter in terms of VecDim so that if you change the representation you only need to change it in one place and the change automatically cascades through the code.

typedef vector VecDim;
typedef VecDim::iterator  VecDimIter;  // And it lines up nicer :-)


Now you try and use shared pointers:

typedef shared_ptr > PVecDim;
                              //    ^^^   This problem should be fixed on all modern
                              //          compilers. You really don't need the space
                              //          anymore.


But there is no need.

The only object of this type you have has a well defined lifetime. Just use VecDim. Then pass the object by reference to your box algorithm.

Please consistent formatting:

for (VecDimIter it = inList->begin(); it != inList->end(); ++it) {
      coutheightlengthwidth<<endl;
  }

      PDim modList = getModList(inList);
//^^^^  Why the extra indent it confused me.


I don't recommend calculating the size of an array:

for (int i=0;i<sizeof(d)/sizeof(d[0]); ++i) {
  //           ^^^^^^^^^^^^^^^^^^^^^^


It is really vulnerable to breakage when your code is modified later (say this part of the code is tucked into a function called init() and d is passed as a parameter). Personally I would define this as a vector and use the size method.

// This does require C++11
// But it makes it less prone to bugs.
std::vector d = {{4, 6, 7}, {1, 2, 3}, {4, 5, 6}, {10, 12, 32}};


PPS. I prefer to name my classes (types) with an initial capitol letter (everything else starts with a lowercase letter). It makes it easy to see type names.

Lots of compilers don't like this:

class dimension;
struct dimension {   // struct but it was a class.


Neither do I. Be consistent. Since it is just a property bag make it struct in both places. Also turn on more warnings so the compiler complains about this.

Code Snippets

typedef vector<dimension*> VecDim;
        //              ^     Pointer
typedef vector<dimension*>::iterator VecDimIter;
        //              ^     Pointer
typedef vector<dimension> VecDim;
typedef vector<dimension>::iterator VecDimIter;
typedef vector<dimension> VecDim;
typedef VecDim::iterator  VecDimIter;  // And it lines up nicer :-)
typedef shared_ptr<vector<dimension*> > PVecDim;
                              //    ^^^   This problem should be fixed on all modern
                              //          compilers. You really don't need the space
                              //          anymore.
for (VecDimIter it = inList->begin(); it != inList->end(); ++it) {
      cout<<(*it)->height<<" "<<(*it)->length<<" "<<(*it)->width<<endl;
  }

      PDim modList = getModList(inList);
//^^^^  Why the extra indent it confused me.

Context

StackExchange Code Review Q#39279, answer score: 6

Revisions (0)

No revisions yet.