5
\$\begingroup\$

We have just learned about classes. Please help me improve this program. I created a class called Box with members that represent its dimensions. Then I calculate the volume and surface area and print it.

#include <iostream>

using std::cout;
using std::endl;

class Box
{
public:
    Box(){};
    double m_Length;
    double m_Width;
    double m_Height;
private:
};

int main()
{
    Box box1;
    Box box2;

    double boxVolume(0.0);
    double surfaceArea(0.0);

    box1.m_Height = 18.0;
    box1.m_Length = 78.0;
    box1.m_Width = 24.0;


    boxVolume = box1.m_Height * box1.m_Length * box1.m_Width;
    surfaceArea = 2.0 * ((box1.m_Height * box1.m_Width) + (box1.m_Length * box1.m_Height) + (box1.m_Length * box1.m_Width));


    cout << "Volume of box1 = " << boxVolume << endl;
    cout << "Surface Area of box1 = " << surfaceArea << endl;
}
\$\endgroup\$
  • \$\begingroup\$ Is this a homework assignment, or something you've created yourself? \$\endgroup\$ – Jamal Oct 12 '14 at 6:16
  • \$\begingroup\$ I wrote this myself. \$\endgroup\$ – user54828 Oct 12 '14 at 6:17
2
\$\begingroup\$
  1. You use the private keyword, yet there are no private data members? Having the dimensions of Box be private enables you to separate the interface to the class from its internal implementation.

    I would suggest having a public member function called volume() that computes the volume of a Box and returns it. And a public member function for calculating surface area. You will also need to supply a constructor.

#include <iostream>

using std::cout;
using std::endl;

class Box
{
public:
    Box(double m_Length = 1, double m_Width = 1, double m_Height = 1);
    double Volume();
    double SurfaceArea();


private:
    double m_Length;
    double m_Width;
    double m_Height;
};

int main()
{
    Box box1(78.0,24.0,18.0); //Initializing box1 data members

    double boxVolume(0.0);
    boxVolume = box1.Volume();


    cout << "Volume of box1 = " << boxVolume << endl;

}

Notice I am defining these functions outside of the class. To do this you need to put the prototype for the function inside the class. You can put whole functions inside your class, but it hurts readability. Inline functions are best used for very short and simple functions.

Box::Box(double m_Length, double m_Width, double m_Height) : m_Length(m_Length), m_Width(m_Width), m_Height(m_Height){

}

double Box::Volume(){
     return m_Length * m_Width * m_Height;
}

double Box::SurfaceArea(){
    return 2.0 * ((m_Height * m_Width) +
                  (m_Length * m_Height) +
                  (m_Length * m_Width));

}
  1. Why are you declaring a box2? It doesn't seem to serve any purpose..

I'm not sure what you have have already learned or haven't learned. So this review is short.

\$\endgroup\$
  • \$\begingroup\$ box2 wasn't supposed to be used, I should have removed it. sorry. \$\endgroup\$ – user54828 Oct 12 '14 at 6:23
  • \$\begingroup\$ The inner curly braces in Volume() are unnecessary. \$\endgroup\$ – Jamal Oct 12 '14 at 6:26
  • 1
    \$\begingroup\$ @Jamal My compiler put them there when it was auto-indenting -.- \$\endgroup\$ – Quaxton Hale Oct 12 '14 at 6:27
4
\$\begingroup\$

The Box class has all public members and doesn't have any behavior (no methods). As such, it's a missed opportunity and could have been a simple struct.

Even worse, what's the point of a Box if any of length or width or height is undefined? These should all be required to construct a meaningful representation of a box.

The calculation of the surface and volume are inside the main method. This is not convenient, because you would have to re-type the formula every time you want to calculate this for every single box.

To remedy the above:

  • make the length, width, height variables private: code outside of the class should not modify them
  • make the length, width, height required at construction time: a box without these parameters doesn't make sense, and is not good modelization
  • I would go as far as making these variables constant: in real life, the width / height / length of a physical box is usually not editable
  • add surface and volume calculation logic as methods on the box, so that they can be reused for any Box object

A better interface for a Box would be:

class Box
{
public:
    Box(double length, double width, double height);
    inline double volume();
    inline double surface();
private:
    const double m_Length;
    const double m_Width;
    const double m_Height;
};

With implementation:

Box::Box(double length, double width, double height) :
    m_Length(length), m_Width(width), m_Height(height) {}

double Box::volume()
{
    return m_Length * m_Width * m_Height;
}

double Box::surface()
{
    return 2 * (m_Length * m_Width + m_Width * m_Height + m_Height * m_Length);
}

With the above, the main method can be rewritten as:

int main()
{
    Box box1(78, 24, 18);

    cout << "Volume of box1 = " << box1.volume() << endl;
    cout << "Surface Area of box1 = " << box1.surface() << endl;
}

Note that since the implementation of the constructor and the surface and volume methods are so simple, it might be tempting to write them inside the declaration of the class, like this:

class Box
{
public:
    Box(double length, double width, double height) : m_Length(length), m_Width(width), m_Height(height) {}
    double volume() { return m_Length * m_Width * m_Height; }
    double surface() { return 2 * (m_Length * m_Width + m_Width * m_Height + m_Height * m_Length); }
private:
    double m_Length;
    double m_Width;
    double m_Height;
};

But this is a not a good practice. The good practice is to cleanly separate the interface declaration from the implementation. However, while you're learning, if this last version is easier for you, then it's ok, you can do it this way, for now. Just remember later as your understanding evolves, to try to separate interface from implementation.


Finally, some other minor issues:

No need to write 18.0 when assigning to a double variable, you could write simply 18.

No need to put parentheses around multiplication in (a * b) + (c * d), a * b + c * d is equivalent.

\$\endgroup\$

Your Answer

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