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 ' DegToRad
andRadToDeg`, 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.
