Triangle class implementation

Question

I need some advice to improve my Triangle class. I know some of my code is sloppy, such as in the Triangle constructor. can someone give me advice on how to improve it?

#pragma once
#include "Shape.h"
#include <algorithm> 
class Triangle :public Shape
{
    std::vector<Point> _vertices;
    float _a, _b, _c, _s;

public:
    Triangle(){}


    Triangle(const std::vector<Point> vertices) : _vertices(vertices)
    {
        _a = sqrt((vertices.at(2).getX() - vertices.at(0).getX()) * (vertices.at(2).getX() - vertices.at(0).getX())) + (vertices.at(2).getY() - vertices.at(0).getY()) * (vertices.at(2).getY() - vertices.at(0).getY());
        _b = sqrt((vertices.at(2).getX() - vertices.at(1).getX()) * (vertices.at(2).getX() - vertices.at(1).getX())) + (vertices.at(2).getY() - vertices.at(1).getY()) * (vertices.at(2).getY() - vertices.at(1).getY());
        _c = sqrt((vertices.at(1).getX() - vertices.at(0).getX()) * (vertices.at(1).getX() - vertices.at(0).getX())) + (vertices.at(1).getY() - vertices.at(0).getY()) * (vertices.at(1).getY() - vertices.at(0).getY());
        _s = (_a + _b + _c) / 2;
    }
    //Triangle(float a, float b, float c, float s, const std::vector<Point> vertices) :_a(a), _b(b), _c(c), _s(s), _vertices(vertices)
    //{
    //  _a = sqrt((vertices.at(2).getX() - vertices.at(0).getX()) * (vertices.at(2).getX() - vertices.at(0).getX())) + (vertices.at(2).getY() - vertices.at(0).getY()) * (vertices.at(2).getY() - vertices.at(0).getY());
    //  _b = sqrt((vertices.at(2).getX() - vertices.at(1).getX()) * (vertices.at(2).getX() - vertices.at(1).getX())) + (vertices.at(2).getY() - vertices.at(1).getY()) * (vertices.at(2).getY() - vertices.at(1).getY());
    //  _c = sqrt((vertices.at(1).getX() - vertices.at(0).getX()) * (vertices.at(1).getX() - vertices.at(0).getX())) + (vertices.at(1).getY() - vertices.at(0).getY()) * (vertices.at(1).getY() - vertices.at(0).getY());
    //  _s = (_a + _b + _c) / 2;
    //}
    void  setA(std::vector<Point> vertices)
    {
        _a = sqrt((vertices.at(2).getX() - vertices.at(0).getX()) * (vertices.at(2).getX() - vertices.at(0).getX())) + (vertices.at(2).getY() - vertices.at(0).getY()) * (vertices.at(2).getY() - vertices.at(0).getY());
    }
    void  setB(std::vector<Point> vertices)
    {
        _b = sqrt((vertices.at(2).getX() - vertices.at(1).getX()) * (vertices.at(2).getX() - vertices.at(1).getX())) + (vertices.at(2).getY() - vertices.at(1).getY()) * (vertices.at(2).getY() - vertices.at(1).getY());
    }

    void  setC(std::vector<Point> vertices)
    {
        _c = sqrt((vertices.at(1).getX() - vertices.at(0).getX()) * (vertices.at(1).getX() - vertices.at(0).getX())) + (vertices.at(1).getY() - vertices.at(0).getY()) * (vertices.at(1).getY() - vertices.at(0).getY());
    }
    void  setS(float _a, float _b, float _c)
    {
        _s = (_a + _b + _c) / 2;
    }


    float getA() const
    {
        return _a;
    }

    float getB() const
    {
        return _b;
    }

    float getC() const
    {
        return _c;
    }

    float getS() const
    {
        return _s;
    }



    ~Triangle(){}

    float Area() const override;
    float Circumference() const override;
    std::vector<Point> BoundingBox() const override;
    void Display() override;

};

#include "Triangle.h"
#include <iostream>


float Triangle::Area() const
{
    return ((_vertices.at(0).getX() * (_vertices.at(1).getY() - _vertices.at(2).getY())) + (_vertices.at(1).getX() * (_vertices.at(2).getY() - _vertices.at(0).getY())) + (_vertices.at(2).getX() * (_vertices.at(0).getY() - _vertices.at(1).getY()))) / 2.0f;
}

float Triangle::Circumference() const
{
    return _a + _b + _c;
}

std::vector<Point> Triangle::BoundingBox() const
{

    float _largeX, _largeY, _smallX, _smallY;
    _largeX = std::max(std::max(_vertices.at(0).getX(), _vertices.at(1).getX()), _vertices.at(2).getX());
    //calculate largest Y
    _largeY = std::max(std::max(_vertices.at(0).getY(), _vertices.at(1).getY()), _vertices.at(2).getY());
    //calculate smallest x
    _smallX = std::min(std::min(_vertices.at(0).getX(), _vertices.at(1).getX()), _vertices.at(2).getX());
    //calculate smallest y
    _smallY = std::min(std::min(_vertices.at(0).getY(), _vertices.at(1).getY()), _vertices.at(2).getY());

    std::vector<Point> boundingArea;
    boundingArea.push_back(Point(_smallX, _largeY));
    boundingArea.push_back(Point(_largeX, _largeY));
    boundingArea.push_back(Point(_smallX, _smallY));
    boundingArea.push_back(Point(_largeX, _smallY));
    return boundingArea;
}

void Triangle::Display()
{
    std::cout << std::endl;
    std::cout << "Triangle: " << std::endl;
    std::cout << std::endl;
    std::cout << "Vertices: ";
    for (auto it : _vertices)
        std::cout << "[" << it << "] ";
    std::cout << std::endl;
    std::cout << "Area: " << Area() << std::endl;
    std::cout << "Perimeter: " << Circumference() << std::endl;
    std::cout << "Bounding Coordinates: ";
    for (auto it : BoundingBox())
        std::cout << "[" << it << "] ";
    std::cout << std::endl;
}

Answer 1

Don't repeat yourself!

Triangle(const std::vector<Point> vertices) : _vertices(vertices)
{
    _a = sqrt((vertices.at(2).getX() - vertices.at(0).getX()) * (vertices.at(2).getX() - vertices.at(0).getX())) + (vertices.at(2).getY() - vertices.at(0).getY()) * (vertices.at(2).getY() - vertices.at(0).getY());
    _b = sqrt((vertices.at(2).getX() - vertices.at(1).getX()) * (vertices.at(2).getX() - vertices.at(1).getX())) + (vertices.at(2).getY() - vertices.at(1).getY()) * (vertices.at(2).getY() - vertices.at(1).getY());
    _c = sqrt((vertices.at(1).getX() - vertices.at(0).getX()) * (vertices.at(1).getX() - vertices.at(0).getX())) + (vertices.at(1).getY() - vertices.at(0).getY()) * (vertices.at(1).getY() - vertices.at(0).getY());
    _s = (_a + _b + _c) / 2;
}

If we have a close look at this code, we'll notice that the very same lines for _a, _b and so on follow later in your code in setA, setB and so on. That's repeated work, and if you ever find an error in setA, you also have to fix it in Triangle::Triangle(...).

Instead, use the already defined functions:

Triangle(const std::vector<Point> vertices) : _vertices(vertices)
{
   setA(vertices);       
   setB(vertices);
   setC(vertices);
   setS(vertices);
}

Note that you shouldn't copy the vector all the time (see below).

Don't use std::vector<...> for fixed size collections

You're using std::vector<Point> in order to get three points into your function. That's error prone. One could use two points and end up with an exception, or four point and don't notice that they actually wanted a rectangle.

Instead, use a fixed size collection. This could be std::array<Point, 3>, or something like

struct TrianglePoints {
    Point p1, p2, p3;        
}

I would prefer the array version though. At the same time, you can give std::array<Point, 3> a better name:

using TrianglePoints = std::array<Point, 3>;

Now you don't have to use at anymore, since the array will have a size of three, no more, no less:

Triangle(const TrianglePoints & vertices) : _vertices(vertices) {
   setA(vertices);
   // ...
}

setA(const TrianglePoints & vertices) {
    std::sqrt((vertices[2].getX() - vertices[0].getX()) * ...
}

However, I think that your Point class should be a struct, as it is probably a quasi class.

Use const T& instead of const T in functions

Doesn't require a copy.

Keep your data in sync

It doesn't make sense to set _a with a bunch of vertices, does it? After all, the values of _a, _b, _c and _s should correspond to the original _vertices, right?

Therefore, you should make setA, setB and so on private and provide only a single updating function

Triangle::update(const TrianglePoints & verts) {
    // …
}

Delete unused functions, default others

The default constructor should be deleted:

Triangle() = deleted;

You need a C++11 compiler for this. Alternatively, move Triangle() into the private part of your class declaration.

The move, assignment and copy operations can be defaulted:

Triangle(const Triangle&)              = default;
Triangle(Triangle &&)                  = default;
Triangle & operator=(const Triangle &) = default;
Triangle & operator=(Triangle &&)      = default;
~Triangle()                            = default;

Irregular names

Some of your methods start with a lower case and a verb, others start with an uppercase. You should probably sort that out.

Use double instead of float

Unless you really want to have single precision only, use double. You can use something like

typedef double coordinate_type;

if you want to change it later.

Answer 2

I'll try to avoid repeating the same points @Zeta already raised, since I think he did a good job.

Names

A circle (or possibly an ellipse, though opinions vary on that) has a circumference. Another closed polygon like a triangle has a perimeter, so that would probably be a better name to use.

A related (but arguably separate) point would be to use operator<< instead of Display. This lets a user specify the destination stream, and returns the stream to let the user chain, find status, etc.

Don't pay for what you don't use

In C++ there's a fairly long tradition that you shouldn't pay for what you don't use. In this case, you pay quite a bit for information you may never use, such as the lengths of the sides. I'd prefer to calculate side lengths on demand, so you only pay for that fairly expensive calculation when/if you actually use it. On the other hand, we also typically want to minimize the cost of those calculations, so we don't want to repeat them unnecessarily.

To accommodate both of those, we'd typically make the lengths of the sides mutable, and either have an invalid value or else a separate bool to indicate whether a length is valid at any given time. In this case, we could (for example) use any negative number as an invalid length.

This way, when the user wants a length of a side, we can just return the length if it's valid, or compute/save/return the length if it's not currently valid.

The triangle's area would be much the same way: compute on demand, save when computed, returned if and only if valid.

In both cases, you need to invalidate the relevant parts when a vertex changes. Changing one vertex invalidates the lengths of both sides that depend on that vertex (but not the opposite side). Changing any vertex invalidates the triangle's area.

In both cases, "invalidates" should really be read as "potentially invalidates", I suppose. At least in theory, it's possible to change a vertex to another that produces the same area, but since it might have changed, you just invalidate the area, and re-compute if needed.

Utility functions

You have a lot of repetition of the code to compute the hypotenuse of a right triangle. I'd prefer to move this into a single function named something like hypot, and just use it.

Avoid .at()

If you're going to use a vector at all, I'd validate that it has a length of 3 when you create the triangle, then use operator[] to index into the vector from then on. Using at(n) every time you want to access a vertex adds quite a bit more overhead without gaining much (of anything).