4
\$\begingroup\$

Experimenting with operator overloading for the first time. Based on my reading, it appears to be a bit of a minefield.

Have I fallen into any traps?

namespace Units
{
    class Degree; // Forward declare Degree so it can be passed by reference into Radian.

    /// \class Radian
    /// Encapsulates a double, and used to represent a radian.
    class Radian
    {

    public:
        /// Constructor.
        /// \param radian The  angle (radians).
        Radian(double radian) : angle_(radian) {}

        /// Constructor.
        /// \param degree The  angle (degrees).
        Radian(const Degree& degree);

        /// Function Call Operator.
        /// \return <double> The angle value (radians).
        operator double() const { return angle_; }

    private:
        double angle_;

    };

    /// \class Degree
    /// Encapsulates a degree, and used to represent a degree.
    class Degree
    {
    public:
        /// Constructor.
        /// \param degree The angle value (degrees).
        Degree(double degree) : angle_(degree) {}

        /// Constructor.
        /// \param radian The  angle (radians).
        Degree(const Radian& radian);

        /// Function Call Operator.
        /// \return <double> The angle (degrees).
        operator double() const { return angle_; }

    private:
        double angle_;
    };

    /// Constructor.
    /// Constructor defined outside of class declaration, to allow Degree class to be defined before it.
    /// \param degree The  angle (degrees).
    Radian::Radian(const Degree& degree)
    : angle_(degree * SoC::Maths::Trigonometry::DegToRad)
    {}

    /// Constructor.
    /// Constructor defined outside of class declaration, to allow Radian class to be defined before it.
    /// \param radian The  angle (radians).
    Degree::Degree(const Radian& radian)
    : angle_(radian * SoC::Maths::Trigonometry::RadToDeg)
    {}

} // namespace Units

#endif // UNITS_H

EDIT

and initialised in the way:

const Units::Degree latitude = 48.8566;
\$\endgroup\$
2
  • 2
    \$\begingroup\$ For a more all-encompassing version of the same general idea, you might want to take a look at Boost Units sometime. \$\endgroup\$ Oct 4, 2019 at 18:03
  • 1
    \$\begingroup\$ The std::chrono library can inspire you. \$\endgroup\$
    – L. F.
    Oct 5, 2019 at 13:26

2 Answers 2

9
\$\begingroup\$

Design

While it might seem that the current design is working fine, I think there are two distinct issues lurking beneath the surface.

Issue #1: operator double

From what I can tell, the intention behind these unit classes seems to be the prevention of unit mismatches. Providing an access function to the contained value is not a bad idea, but using the implicit conversion operator for doing so is probably not the wisest choice here.

Consider the following snippet:

auto angle = Degrees{ 180.0 };
auto sine = std::sin(angle);

It compiles easy enough, but will then fail silently at runtime. It likely won't even crash the application, but quietly produce values different to those expected.

Of course, this is a contrived case ("std::sin is not really in the scope of this library (yet)!"), but nonetheless it shows a problem: As it is, there is hardly any prevention of accidental unit mismatches.

Adding the keyword explicit to operator double() might help with some of these cases (though not with std::sin), but not all of them.

If this were the only issue, it would be easy to fix with a getter function with a descriptive name (e.g. getDegrees).

Issue #2: Extensibility

Let's say that in the future, you (as the library developer) want to add another representation for angles (e.g. gons/gradians). Sounds simple, right?

And at that point, it is. Adding one more class according to the given scheme, four more converting constructors, and it is done.

Someone familiar with the SOLID principles might already spot a code smell in the last sentence: four more converting constructors, two of which have to be added to the existing and otherwise rather independent classes Degrees and Radians, thus violating the Open-Closed part of SOLID.

After that comes another user A of the library and wants to add his own custom angle representation RepA. And then comes user B with RepB.

And suddenly, we're having twenty converting constructors just for those five classes. And each additional representation is going to add a lot more: For \$N\$ representations, we need \$N \cdot (N - 1)\$ converting constructors to cover all combinations.

And that is assuming independent developers add converting constructors for each others implementation. Otherwise, operator double will again lurk in the shadows, allowing for code to compile that really should not.

class Gradians {
public:
    Gradians(double);
    Gradians(const Degrees&);
    Gradians(const Radians&);
    operator double() const;
    // ...
};

class Turns {
public:
    Turns(double);
    Turns(const Degrees&);
    Turns(const Radians&);
    operator double() const;
    // ...
};

Now stuff like auto a = Gons(300.0); auto b = Turns(a); will actually compile, but produce wrong results (b == 300.0 instead of b == 0.75).

How can we solve this conundrum?

A first step would be to separate the value from its representation(s) by choosing one internal representation which can be converted on demand:

class Angle {
public:
    static Angle fromDegrees(double degrees);
    static Angle fromRadians(double radians);

    double radians() const;
    double degrees() const;
private:
    Angle(double radians) : radians_{ radians } {}

    double radians_;
};

Angle Angle::fromDegrees(double degrees) {
    auto radians = degrees * SoC::Maths::Trigonometry::DegToRad;
    return Angle{ radians };
}

Angle Angle::fromRadians(double radians) {
    return Angle{ radians };
}

double Angle::degrees() const {
    return radians_ * SoC::Maths::Trigonometry::RadToDeg;
}

double Angle::radians() const {
    return radians_;
}

As you can see, I chose radians for my internal representation (mostly because that's what the trigonometric functions of the standard libary expect). For adding a new representation, we now only need to add one factory function (fromXyz(...)) and one getter function (xyz()).

While this is a lot cleaner (and takes care of some issues), SOLID devotees will not fail to notice that the violation of the Open-Closed principle hasn't been fixed yet, just moved.

To address this, we could introduce a hierarchy of derived classes, but that seems like overkill for this problem.

Another easy solution would be to use templates:

struct Degrees {
    static double toRadians(double degrees) {
        return degrees * SoC::Maths::Trigonometry::DegToRad;
    }
    static double fromRadians(double radians) {
        return radians * SoC::Maths::Trigonometry::RadToDeg;
    }
};

struct Radians {
    static double toRadians(double radians) { return radians; }
    static double fromRadians(double radians) { return radians; }
};

class Angle {
public:
    template<typename Representation>
    static Angle from(double value) {
        return Angle{ Representation::toRadian(value) };
    }

    template<typename Representation>
    double as() const {
        return Repreentation::fromRadians(radians_);
    }

private:
    Angle(double radians) : radians_{ radians } {}

    double radians_;
};

 // Usage
 auto angle = Angle::from<Degrees>(180.0);
 auto sine = std::sin(angle.as<Radians>());

Of course, this is far from done, yet:

  • Operators for addition, subtraction (angles), multiplication and/or division (scalars) could be overloaded for this Angle class
  • For demonstration purposes I didn't mark the member functions above noexcept or constexpr. This should likely be amended.
  • Helper functions like sin, cos, tan and similar could be provided for this Angle class.
  • For the template version: The templates could be restricted to only accept types with correct signatures for fromRadians and toRadians.

Implementation

Aside from the design considerations mentioned above, I can add these points for the general implementation:

  • Consider marking converting constructors and conversion operators as explicit.
  • Very likely sizeof(Degree) == sizeof(double), so there probable won't be a benefit for taking a const Degree& parameter over just Degree.
  • I'd suggest checking the precision of the constants ' DegToRadandRadToDeg`, especially if calculated on your own. If the precision on these constants is poor, there might be small numeric errors that accumulate over multiple conversions to and fro.
  • A comment reads /// Function Call Operator: Actually, no, this is a conversion operator. A function call operator would look like this: double operator()() const.
  • Generally, the comments don't tell me much about anything. Unless there is a hard requirement for them (in which case they should be improved) I'd suggest removing them. In their current form, they are at best visual clutter, and confusing at worst.
\$\endgroup\$
7
  • \$\begingroup\$ I really appreciate the time you took to review my code! Thank you. I did consider using an Angle class, however dismissed it. Currently, I am passing round doubles to represent my angles, however this has lead to much confusion as to whether a function is operating with degrees or radians. My intention with these classes was to remove that ambiguity, and I'm not sure an "Angle" class would do that. I acknowledge the issues with using an implicit conversion. Is there any other way to allow assignments as per my edit? Or am I wrong to want to be able to do this? \$\endgroup\$
    – pingu
    Oct 5, 2019 at 12:09
  • 1
    \$\begingroup\$ @pingu The intention behind the Angle class is that you should not need to care about the angle's representation until you explicitly request one. A function taking an Angle parameter doesn't care whether the Angle was initialized from degrees, radians, gons or something else. And if it actually needs the value in a specific representation, it can request it, and it will be converted automatically. (Hint: This might be much easier to use if some of the arithmetic operators are overloaded: 2*alpha is double the angle, regardless of the representation) \$\endgroup\$
    – hoffmale
    Oct 5, 2019 at 13:59
  • 1
    \$\begingroup\$ @pingu: A bit more worked out: godbolt link. Take a look at myFunc at the bottom: It doesn't care which representation was used to create its inputs, just does some calculations on the angle and passes it on. Only sin actually cares about the representation, and then it actually requests it as needed. If you only pass Angles around, you mostly won't even need to care about the actual internal representation, so in my eyes, this is definitely an improvement \$\endgroup\$
    – hoffmale
    Oct 5, 2019 at 14:28
  • \$\begingroup\$ I'd suggest that one could also implement user-defined literals as suggested in this answer \$\endgroup\$
    – Edward
    Oct 29, 2019 at 21:50
  • \$\begingroup\$ @pingu You say, "I am passing round doubles to represent my angles, however this has lead to much confusion as to whether a function is operating with degrees or radians." Is there a reason you don't have a convention like the standard library that all functions dealing with angles take radians? Seems like that would solve the issue. Internally a function could do whatever it wants, but externally all angles should be the same representation, usually. \$\endgroup\$ Oct 30, 2019 at 0:47
3
\$\begingroup\$

The two conversion constructors are not declared in the class, nor are they declared inline. If this code is in a header that is included by multiple source files, you'll have an ODR violation, which typically results in a linker error for multiple definitions of a symbol.

Those two constructors should be declared with the inline keyword.

inline Radian::Radian(const Degree& degree)
// ...

inline Degree::Degree(const Radian& radian)
// ...
\$\endgroup\$
1
  • \$\begingroup\$ many thanks, I will make these changes, and I have learned something. \$\endgroup\$
    – pingu
    Oct 5, 2019 at 12:10

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.