Basic C++ Circle Class

Question

I'm trying to improve on my basic C++ skills and create really nice, concise and clean code that is free from artifacts.

The program can add and subtract radius values from each circle, assign one circle to another and furthermore give the area.

Any tips on how this program can be improved, whether considering readability issue or anything else would be great.

//  Basic circle exercise 23rd March

#include <cstdio>
#include <math.h>

using namespace std;

//==============================================================================
/*                               interface                                    */
//==============================================================================
namespace corey
{
    class Circle
    {
    public:
        Circle(const int & r = 1.0);
        ~Circle();

        const double & getRadius() const;
        const double getArea() const;

        Circle & operator = (const Circle &);
    private:
        double radius;
    };
};


//==============================================================================
/*                              implementation                                */
//==============================================================================
/*I'd probably included using namespace corey if
    i'd have bothered with seperate files*/

corey::Circle::Circle(const int & r) : radius(r){}

corey::Circle::~Circle(){}

const double & corey::Circle::getRadius() const
{
    return radius;
}

const double corey::Circle::getArea() const
{
    const double area = (M_PI * radius * radius);
    return area;
}

corey::Circle operator + ( const corey::Circle & lhs, const corey::Circle & rhs)
{
    return corey::Circle(lhs.getRadius() + rhs.getRadius());
}

corey::Circle operator - ( const corey::Circle & lhs, const corey::Circle & rhs)
{
    return corey::Circle(lhs.getRadius() - rhs.getRadius());
}

corey::Circle & corey::Circle::operator = (const Circle & lhs)
{
    if(&lhs != this)
    {
        radius = lhs.radius;
    }
    return (*this);
}


//==============================================================================
/*                                   useage                                   */
//==============================================================================
int main( int argc, char ** argv )
{
    //this stuff is just some info printed out so not super important...
    corey::Circle c1(2);
    printf("Circle made with radius %lf & area %lf\n", c1.getRadius(), c1.getArea());

    const corey::Circle c2(6);
    printf("Circle made with radius %lf & area %lf\n", c2.getRadius(), c2.getArea());

    corey::Circle c3;
    c3 = c2 + c1;
    printf("Circle made with radius %lf & area %lf\n", c3.getRadius(), c3.getArea());

    c1 = c3;
    printf("Circle made with radius %lf & area %lf\n", c1.getRadius(), c1.getArea());

    return 0;
}

Answer 1

I think this is a good start. It's straightforward and easy to read. The data is immutable, which makes it less error prone. Here are some things I would change.

Is It Complete?

I notice that your circle doesn't have a location. Why not? In most of the code that I've dealt with where I have a shape, there's almost always a location or position for where the circle is in space. This is usually necessary because I often need to draw it, intersect it, hit test against it, etc. I would suggest creating a Point or Coordinate class and using an object of that class as the location of the Circle.

Careful With Overloads

The ability to overload the basic math operators is a nice feature of C++. However, it can be confusing if the meaning of those overloads isn't obvious from looking at the class definition. It's not immediately obvious to me what it means to add 2 circles. There are places where adding and subtracting shapes makes sense (constructive solid geometry), but the results are very different from what's happening here. I think the overloads you have are likely to confuse future readers of the code. I would make regular methods with names like addRadiuses() and subtractRadiuses() instead.

Avoid using namespace std

It's been said frequently that one should avoid using using namespace std. I'll let the experts explain it.

I also don't think naming a namespace after yourself is too useful. The name of a namespace should tell what it's for and hint at what it might contain. I don't know what corey should contain.

Answer 2

Be consistent with #include

We have C++ <cstdio> (which defines the C names in the std namespace), but the C-style <math.h> (which puts the identifiers in the global namespace). For new C++ code, prefer <cmath>. And don't use using namespace std; - that can be harmful.

Allow the compiler to define assignment

We don't need to do anything in operator=() that's different from the compiler-generated (member-by-member copy) assignment. Declaring our own actually prevents some optimizations that can be done for so-called "trivial" types (without the operator, the compiler knows it can simply copy the object's memory, but with one, it must assume that copying is more involved than that; it also inhibits the generation of a move assignment operator).

Similarly, we don't need to define a destructor - let the compiler generate that, too.

Pass by value for primitive types

There's no benefit to passing a reference to const int in the constructor - just pass the int by value. That's simpler and (before optimization) faster.

Actually, why is this an int, when we could accept any double value? And why is the default 1.0? I think that 0.0 (an empty circle) would be less surprising (it's then more like the numbers, where default initialization uses zero). Consider also some validation - is a negative radius meaningful for your code?

It's better to return double by value from getRadius() for much the same reasons.

Mark the constructor explicit

Unless you like surprises, it's better if you don't allow numbers to convert automatically to circles.

Don't return 'const' values

The const double return type of getArea() is no different to plain double - the const will be ignored (and that's a good thing). So it's useless clutter to write it (and GCC will warn you, assuming you turn on a reasonable set of warnings).

Operators

What does it mean to subtract one circle from another? In a geometry library, I'd want to get an annulus back (i.e. a circle with a hole cut out) or not be allowed to subtract at all. Similarly for addition.

If you do choose to provide operators, you'll want to declare them with your interface (so this could be moved to a header). And you should consider whether to provide the corresponding assignment operators (+= and -=).

On the positive, you have the correct argument and return types here.

Portability

Your environment clearly provides you with M_PI for a value of π. That's not a standard identifier, and although it's a common extension, it's more portable to define your own. Popular expressions producing π include 4 * std::atan(1) and std::acos(-1) These are both compile-time constants, so cost nothing to the program's users.

A thing I like in the getArea() is that you write radius * radius and didn't get seduced by std::pow(radius, 2) - which is generally slower and less accurate than multiplication (it's more versatile, because it supports fractional powers, but we don't need that here).

Prefer C++ formatted output

The <iostream> header provides I/O with better type-checking than the C-style <cstdio> functions. Consider providing a << operator to stream to a standard library stream.

---

Modified code

#include <cmath>
#include <iosfwd>

namespace corey
{
    class Circle
    {
        static constexpr double pi = 4 * std::atan(1);
        double radius;

    public:
        explicit Circle(double r = 0);

        double getRadius() const;
        double getArea() const;
    };

    Circle operator+(const Circle&, const Circle&);
    Circle operator-(const Circle&, const Circle&);

    std::ostream& operator<<(std::ostream&, const Circle&);
}

#include <ostream>

namespace corey {
    Circle::Circle(double r)
        : radius(r)
    {}

    double Circle::getRadius() const
    {
        return radius;
    }

    double Circle::getArea() const
    {
        return pi * radius * radius;
    }

    Circle operator+(const Circle& lhs, const Circle& rhs)
    {
        return Circle(lhs.getRadius() + rhs.getRadius());
    }

    Circle operator-(const Circle& lhs, const Circle& rhs)
    {
        return Circle(lhs.getRadius() - rhs.getRadius());
    }

    std::ostream& operator<<(std::ostream& os, const Circle& c)
    {
        return os << "Circle made with radius " << c.getRadius()
                  << " & area " << c.getArea();
    }
}

#include <iostream>

int main()
{
    using corey::Circle;

    Circle c1(2);
    std::cout << c1 << '\n';

    const Circle c2(6);
    std::cout << c2 << '\n';

    Circle c3;
    c3 = c2 + c1;
    std::cout << c3 << '\n';

    c1 = c3;
    std::cout << c1 << '\n';
}

Answer 3

I subscribe to what @user1118321 and @AJD said, but want to add a few remarks.

About references

References aren't always the way to go. In your constructor (Circle(const int & r = 1.0);), it isn't optimal to get the argument by reference, because an int will be smaller than a reference (a pointer under the hood), generally 4 bytes vs 8 bytes. So it'd be better to pass it by value: Circle(int r = 1). By the way, 1.0 is a float, so the assignment looks a bit weird.

Idem, as a return value: const double & corey::Circle::getRadius() const. A const reference will act as a value, so return by value is better (and more idiomatic): double Circle::getRadius() const.

About encapsulation

You probably have read about data encapsulation, that principle according which the class, and the class alone is responsible for handling its data members. Your design is built around this principle.

Don't take such things too literally. In this case, it makes your interface difficult to use without any safety benefit. If I want to modify the radius of my circle (which sounds legitimate), I have to assign it a new circle with the desired radius. That is unefficient and cumbersome.

You should rather start from what functionality you want to offer, and derive your implementation from it. If your circle is defined only by its radius, it's basically a double. And I can sum up all of your code by:

using Circle = double; 
double area(Circle radius) { return M_PI * radius * radius; }
// I get all the rest for free since +, =, etc. are already defined for double

About constness

I don't see any reason to make every circle behave like it is const. Client code might legitimately wish to differentiate const circles and those which aren't.

You can provide for this by implementing two member functions:

double  radius() const { return radius; }
double& radius()       { return radius; } // will be called if `this` isn't const

Or, you can simplify more radically your design and implement Circle as a struct: struct Circle { double radius; }; seems good enough, at least until you have a good reason not to be satisfied with it.

About case

There aren't hard rules about it, but get_radius is more C++-like than getRadius

Answer 4

Default value for constructor.

Why does this produce a circle of radius 1 by default if the constructor has no parameters? If the user hasn't specified the radius, they shouldn't get a result, surely?

Best practice here would be to make the default constructor (with no parameters) private. If the calling code doesn't set the radius, it simply won't compile.

No copy constructor.

In this case it doesn't matter, because all you're doing is copying the value of radius. The default copy constructor (which does a direct copy of the value of every member) will be fine here. However if you allocated memory at any point, the default copy constructor goes very badly wrong because you can end up with two instances pointing at the same internal data. When one deletes that data, the program will crash when the other one tries to access the data which is no longer there.

getRadius return type.

Why is this returning a reference to double, instead of just double?

Documentation.

I know you're just starting out in C++, and this is just a trial run class, but it's good practice to document your class interface (and internal methods/data if relevant). I suggest you look into Doxygen, which is generally the best-practise way of documenting C and C++ code.

Avoid using C-style stdio in C++.

It's not a big deal, but C++ style is generally to prefer streaming for I/O. std::cout is generally preferred to printf.

Answer 5

I am out of practice in C++, so won't comment on code construct. But I will echo what user1118321 said about being 'complete'. I have created my own Circle class in VB.Net for a geospatial project. These are the methods and functions.

Public Class Circle
    Private p_radius_sq As Decimal
    Private p_Centre As PointD 'Double - based user type for coordinates
    Private p_valid As Boolean ' if the construction fails for geometric reasons, the circle is invalid
    Private p_geometryhelper As New Geometry 'my own class of useful geometry functions
    Private p_rect As RectangleF

' Create circle from a point and a radius
    Public Sub New(Centre As PointD, radius As Decimal)

' Create a circle from three points
    Public Sub New(a As PointD, b As PointD, c As PointD)

' Creates a minimum bounding circle for a list of points.
    Public Sub New(ThePoints As List(Of PointD))

' Useful property for real-world calculations
    Public Property Radius_Squared() As Double

    Public Property Centre() As PointD

    Public ReadOnly Property Radius() As Double

    Public Property Valid() As Boolean

' Identify where one circle intersects with another.
    Public Function IntersectionPoints(OtherCircle As Circle) As List(Of PointD)

' Identifies if a set of points is enclosed by this circle
    Private Function EnclosesPoints(points As List(Of PointD), Optional skip1 As Integer = -1, Optional skip2 As Integer = -1, Optional skip3 As Integer = -1) As Boolean

' See if the circle encloses the nominated point
    Public Function EnclosesPoint(ThePt As PointD) As Boolean

End Class

The PointD and geometry helpers round out a series of useful functions for a powerful circle tool. In my application, I needed the Decimal type to get the precision (Double was not precise enough).

Answer 6

Put implementation code inside the namespace too

Instead of using a using directive, or littering your code with corey::s, the best thing to do is encase the whole implementation inside the same namespace, e.g.

namespace corey {
    Circle::Circle() { ... }
}

This keeps your namespaces relevant, but also keeps your code clean.

Answer 7

If you are learning C++, then you should start using qualifiers immediately, so that it becomes second nature. There are 3 notable ones in your case:

1. const You are already using it great

2. noexcept This tells the compiler that this function cannot throw. Consequently the compiler can simplify exception handling around this function. There are 6 functions for your cycle, of which only 2 are guaranteed to be noexept. Can you tell which and why?

3. constexpr This applies to compile time evaluation. This is a rather complex topic, but you should definitely read up on it as soon as possible, as thedirection in C++ is to go as much as possible towards constexpr.

   Your constructor can be constexpr. This turns your Circle class into a literal. You can also apply constexpr to all the other functions except for the ostream operator. Please inform yourself about the relation between noexcept and constexpr.

4. For completeness sake there are also the following additional qualifiers that should always be used when applicable: final and overload. However they are only relevant with respect to virtual functions.