patterncppMinor
Box-stacking problem in C++
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) {
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:
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.
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.
Now you try and use shared pointers:
But there is no need.
The only object of this type you have has a well defined lifetime. Just use
Please consistent formatting:
I don't recommend calculating the size of an array:
It is really vulnerable to breakage when your code is modified later (say this part of the code is tucked into a function called
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:
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.
typedef vector VecDim;
// ^ Pointer
typedef vector::iterator VecDimIter;
// ^ PointerIts 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;
// ^ Pointertypedef 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.