Area calculator for shapes as an OOP interview test

Question

A while ago a tricky C++ interview question came across to me, and ever since I could not tell what I did wrong.

Create a base class named “Shape” with a method to return the area of the shape. Create a class named “Triangle” derived from the base class “Shape”.

Class “Triangle” must have two constructors. One receives the length of the (a) side of the triangle and the related (ma) height. The other receives the length of (a),(b),(c) sides of the triangle. In the second case, the constructor must validate the input by checking that the length of one side is smaller than the sum of the lengths of the other two sides. If the input is invalid, it should throw an exception. Implement the method (available in the base class) that calculates the area of the triangle on the basis of the available data.

You can use two formulas. If the length of a side and height is given: T=(a*ma)/2. If the lengths of the three sides are given, you can use the Heron formula: sqrt(s*(s-a)*(s-b)*(s-c)) where s=(a+b+c)/2

This should be obvious for any candidate. It clearly asks if you know what the base principles of OOP is, and if you can solve a bit complex problem with it. Before I could just jump into it, I found something odd. The assessment requires you to create some class that behaves differently as they've created. It asks you to use some trickery to solve this problem.

Well, I, who read a bit about what SOLID is, and familiar with design patterns were trivial that using polymorphism is the way to solve this the most elegant way. My approach was something like this:

• Create the base class
• Create the derived class
• Use the derived class constructor as an abstract factory for a composite type, and make the implementation to act as a proxy toward the different implementations
• Create two other classes which handles the actual data and takes care of the calculation
• Take care of the exceptions using std::exception - these are tricky as well in C++
• Take care of the implementation

In the end my solution looked like this:

#include <exception>
#include <string>
#include <cmath>

class Shape
{
public:
    virtual double Area() = 0;
};

class Exception : public std::exception
{
public:
    Exception(std::string const &message) throw() : m_msg(message) {}
    ~Exception() throw() {}
    virtual char const *what() const _NOEXCEPT { return m_msg.c_str(); }

private:
    std::string m_msg;
};

class Triangle : public Shape
{
    class TriangleSides;
    class TriangleBase;

public:
    Triangle(double base, double height);
    Triangle(double a, double b, double c);
    ~Triangle();
    double Area() override;

private:
    Shape *m_shape;

    class TriangleSides : public Shape
    {
    public:
        TriangleSides(double a, double b, double c);
        double Area() override;

    private:
        double m_a, m_b, m_c;
    };

    class TriangleBase : public Shape
    {
    public:
        TriangleBase(double base, double height);
        double Area() override;

    private:
        double m_base, m_height;
    };
};

Triangle::Triangle(double base, double height) : m_shape(nullptr)
{
    m_shape = new Triangle::TriangleBase(base, height);
}

Triangle::Triangle(double a, double b, double c) : m_shape(nullptr)
{
    if (a + b <= c || a + c <= b || b + c <= a)
        throw Exception("Any two sides of a triangle must be longer than one");
    m_shape = new Triangle::TriangleSides(a, b, c);
}

Triangle::~Triangle()
{
    delete m_shape;
}

double Triangle::Area()
{
    if (m_shape)
        return m_shape->Area();
    else
        return 0;
}

Triangle::TriangleSides::TriangleSides(double a, double b, double c) : m_a(a), m_b(b), m_c(c)
{
}

double Triangle::TriangleSides::Area()
{
    const double s = (m_a + m_b + m_c) * .5;
    return sqrt(s * (s - m_a) * (s - m_b) * (s - m_c));
}

Triangle::TriangleBase::TriangleBase(double base, double height) : m_base(base), m_height(height)
{
}

double Triangle::TriangleBase::Area()
{
    return .5 * (m_base * m_height);
}

Answer 1

For a junior hire, I may have accepted the answer, but I would have pointed out the short comings while discussing the answer, and see if the candidate could adjust the code accordingly.

I agree with Edward's assessment that you should use std::domain_error for the exception, and that the Area method should be const.

Missing Virtual Destructor

As a matter of C++ OOP hygiene, all base classes with virtual methods should also include a virtual destructor. Without it, if framework code destroyed the Shape, the destructor of the derived class would not get called.

struct Shape {
    virtual double Area() const = 0;
    virtual ~Shape () = default;
};

It is true that a virtual destructor may incur some additional overhead. But if the framework code needs to destroy polymorphic types, solutions avoiding the virtual destructor are much more cumbersome to implement.

Rule of Three

The proposed solution uses new to dynamically allocate the chosen implementation object which may differ based on which constructor was invoked. However, your implementation violates the Rule of Three. In your case, this means it is not safe to copy, nor assign to, a Triangle.

The Rule of Three is that if you implement one of destructor, copy constructor, or assignment operator, you most likely need to implement all three.

While discussing the proposed solution, I might ask the candidate what might happen if the Triangle object was copy constructed, and then both the copy and the original are destructed. Given the current design, I would expect the candidate to determine that a proper copy constructor that performs a deep copy would have to be implemented.

A similar question regarding assignment would be asked if the candidate did not realize a somewhat similar problem occurs if a Triangle is assigned to a different Triangle (which would also induce a memory leak). Given the current design, I would expect the candidate to determine that a proper assignment operator that frees the existing pointer and then performs a deep copy would have to be implemented.

This might then lead to a question about the copy and swap idiom for assignment operators, which might lead to questions about how to implement a proper swap function for a class.

Rule of Zero

Another way to obey the Rule of Three is to rework your Triangle so that it does not need to implement a destructor. I might challenge the candidate to maintain the dynamic allocation, but lose the destructor without leaking memory. I would expect the candidate to come up with using a single element container or, more conventionally, a smart pointer.

Avoid new

However, another way to avoid implementing a destructor would be to avoid dynamic allocation altogether. Edward's answer is a clear technique to achieving this. However, if you want the area to be computed when needed rather than during construction, then you just need some method to let the area calculation logic choose the right formula.

• Use lambdas and std::function
  This is a close cousin of your choice to use polymorphism, minus the complexity of actually defining derived classes. Instead, you define lambdas within your constructor that get stashed into the Triangle, and invoked when the area is queried.

  class Triangle : Shape {
      std::function<double()> area_;
      static double area(double a, double am) { return (a * am)/2; }
      static double area(double a, double b, double c) {
          double s = (a + b + c)/2;
          return std::sqrt(s*(s-a)*(s-b)*(s-c));
      }
  public:
      Triangle(double base, double height)
      : area_{[base,height](){return area(base,height);}} {}
      Triangle(double a, double b, double c)
      : area_{[a,b,c](){return area(a,b,c);}} {
          double s = a + b + c;
          if ((s > 2*a) && (s > 2*b) && (s > 2*c)) return;
          throw std::domain_error(
              "Any two sides of a triangle must be longer than one");
      }
      double Area() const override { return area_();}
  };
  

• Use an if check
  Change your area calculation logic to decide which formula to use. You don't need to store a flag for such a check. Instead, you can query how many inputs are available.

  class Triangle : Shape {
      std::vector<double> in_;
      static double area(double a, double am) { return (a * am)/2; }
      static double area(double a, double b, double c) {
          double s = (a + b + c)/2;
          return std::sqrt(s*(s-a)*(s-b)*(s-c));
      }
  public:
      Triangle(double base, double height)
      : in_{base, height} {}
      Triangle(double a, double b, double c)
      : in_{a, b, c} {
          double s = a + b + c;
          if ((s > 2*a) && (s > 2*b) && (s > 2*c)) return;
          throw std::domain_error(
              "Any two sides of a triangle must be longer than one");
      }
      double Area() const override {
          return (in_.size() < 3
                  ? area(in_[0], in_[1])
                  : area(in_[0], in_[1], in_[2]));
      }
  };
  

What is Triangle?

From the formulation of the question, it is not clear if Triangle is meant to represent a Shape. I would hope the candidate would point out that the specification of a base and height does not actually define a triangle, but a class of triangles that have a side with a specific length, and each having the same area. If the outcome of this question is that the Triangle class is only ever used to compute an area, then Edward's approach is absolutely the correct one to use.

Negative Inputs?

It is unclear if the asker intended to allow negative inputs. If negative inputs themselves do not invalidate the input, then absolute values would have to be used to validate the sum of lengths and to compute the area.

Answer 2

I see some things that may help you improve your code.

Don't overcomplicate your code

The only thing required of the Shape and Triangle classes is the calculation of area. For that reason, the code can be considerably simplified if the constructor calculates and stores the area.

Use const where practical

The Area() function probably shouldn't alter the underlying Shape object, so it makes sense to declare it const.

Use a standard exception where appropriate

There's really nothing, well... exceptional about the Exception class, so it seems not really to be required. Maybe std::domain_error could be used instead like this:

throw std::domain_error("Any two sides of a triangle must be longer than one");

Provide a test driver

This is more about getting a good review rather than the code itself, but it's often good to give reviewers a complete, compileable example to allow them to see the code in context. Here's what I used:

#include <iostream>
int main() {
    Triangle t1{3, 4, 5};
    std::cout << t1.Area() << '\n';
    try {
        Triangle t2{3, 4, 15};
    }
    catch (std::exception &e) {
        std::cout << e.what() << '\n';
    }
    Triangle t3{3, 4};
    std::cout << t3.Area() << '\n';
}

Use std:: for math functions

The sqrt function is often in the global namespace, but it's required to be in the std namespace. For that reason, it's probably safer to write std::sqrt than just sqrt.

Use standard keywords

Instead of _NOEXCEPT which is not standard, the code could use noexcept.

Reworked example

#include <string>
#include <cmath>
#include <stdexcept>

class Shape
{
public:
    virtual double Area() const = 0;
};

class Triangle : public Shape
{
public:
    Triangle(double base, double height);
    Triangle(double a, double b, double c);
    double Area() const override { return area;}

private:
    double area;
};

Triangle::Triangle(double base, double height) 
    : area{(base * height)/2}
{
}

Triangle::Triangle(double a, double b, double c) 
    : area{(a-b-c)*(-a+b-c)*(a+b-c)*(a+b+c)/16}
{
    if (area < 0)
        throw std::domain_error("Any two sides of a triangle must be longer than one");
    area = std::sqrt(area);
}