14
\$\begingroup\$

I would like to ask about the code below:

[System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)]
public struct AngleDeg
{
    public readonly double Degrees; // immutable struct

    public AngleDeg(double degrees = 0)
    {
        Degrees = degrees;
    }

    public override string ToString()
    {
        return $"{Degrees:F2}°";
    }

    #region Conversion

    public static implicit operator AngleDeg(double rawAngleInDeg)
    {
        return new AngleDeg(rawAngleInDeg);
    }

    public static implicit operator double(AngleDeg v)
    {
        return v.Degrees;
    }

    // TODO: Implement arithmetic operations with operator overloads

    [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
    public AngleRad ToRadians()
    {
        // I could have used an explicit cast operator overload, but I think that a simple cast is not verbose/indicative enough.
        return new AngleRad(Degrees / 180.0 * System.Math.PI);
    }

    #endregion
}

[System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)]
public struct AngleRad
{
    public readonly double Radians; // immutable struct

    public AngleRad(double radians = 0)
    {
        Radians = radians;
    }

    public override string ToString()
    {
        // I want the debugger to display angles in rounded degrees. Much easier to picture angles-in-degrees in my head
        return $"{Radians / System.Math.PI * 180:F2}°";
    }

    #region Conversion

    public static implicit operator AngleRad(double rawAngleInRad)
    {
        return new AngleRad(rawAngleInRad);
    }

    public static implicit operator double(AngleRad v)
    {
        return v.Radians;
    }

    // TODO: Implement arithmetic operations with operator overloads

    [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
    public AngleDeg ToDegrees()
    {
        // I could have used an explicit cast operator overload, but I think that a simple cast is not verbose/indicative enough.
        return new AngleDeg(Radians / System.Math.PI * 180);
    }

    #endregion
}

We have tons of geometry related code in our codebase with lots of calculations including angles. We use double-precision floating point numbers to represent angles right now.

We usually measure angles in radians during the calculations, mainly because they all boil down to System.Math's static methods which expect angles to be in radians. However, it is a nuisance to debug such a code, because a double in the range of 0 and 2PI is nowhere near as informative as an angle in the range of 0 and 360°. Also when these angles displayed to a user, or a user has to type in an angle we always have to use degrees. A conversion must happen somewhere between the GUI-related layers and the maths calculation layers.

These are the pros I can think of:

  • ToString() override always displays angle in degrees (great help for the debugger)
  • Compile time checking of incommensurate angles on the boundaries of code using degrees and code using radians.
  • Can write some handy utility methods, like ClampInRangeOf0And360Degrees().

Possible cons:

  • Performance cost? Is there any?
  • All codes using these structs will have a dependency on them. Using System.Double is maybe more 'reusable code'

The question is: What do you think of the possible drawbacks, pitfalls if we used these structs in a performance critical application? What about readability and code smell?

\$\endgroup\$
  • 5
    \$\begingroup\$ Highly recommend not abbreviating AngleDeg and AngleRad. Are you really saving anything from doing this? Most people would just type AngleD and hit tab anyways, so AngleDegree/AngleRadian reads properly and doesn't change anything in that case. \$\endgroup\$ – Shelby115 Jun 6 '18 at 12:28
  • 4
    \$\begingroup\$ This feels like over kill. I would have one Angle class with a double Value property and a Type enum for either Degrees or Radians. \$\endgroup\$ – Nkosi Jun 6 '18 at 13:00
  • 2
    \$\begingroup\$ Rather than ToDegrees() and ToRadians() in each others' structs, why not have an implicit conversion to/from as you do with double? \$\endgroup\$ – Jesse C. Slicer Jun 6 '18 at 13:19
  • 1
    \$\begingroup\$ in the range of 0 and 2PI [...] in the range of 0 and 360°Do you allow angles to be negative? \$\endgroup\$ – Raimund Krämer Jun 6 '18 at 13:34
8
\$\begingroup\$

IMO you should always use radians internally. Therefore a simple double should be appropriate as type for angle measurements. The only times you should want to convert to degrees, are in the UI. So you'll have to make functionality to convert between radians and degrees (for instance an extension method to double).

A good UI that displays angle measurements should let the user be able to set the format (choose between degrees and radians), because in some situations it is in fact more convenient to view angles in radians. The UI should be able to handle angle input in both degrees and radians according to a general setting.

So, although your structs seem well designed (an immutable struct is a good choice) they are in my opinion more or less redundant and may cause confusion among the developers about what to choose.


When it comes to debugging, you could maybe benefit from the possibilities to customize the display by using the DebuggerDisplayAttribute

\$\endgroup\$
  • 1
    \$\begingroup\$ I would however argue that reading code like Angle toWaypoint is more clean/descriptive than double angleToWaypoint, but agree that having two structs might be confusing. \$\endgroup\$ – D. Ben Knoble Jun 6 '18 at 14:07
  • \$\begingroup\$ @D.BenKnoble: You may have a point. But you can use the same argument on length and other measurements: Length lengthToPoint and then it could be a mess IMO. \$\endgroup\$ – Henrik Hansen Jun 6 '18 at 15:15
  • \$\begingroup\$ Fair point Henrik \$\endgroup\$ – D. Ben Knoble Jun 6 '18 at 15:15
  • 1
    \$\begingroup\$ You could always just override ToString for the debugger as well. \$\endgroup\$ – Shelby115 Jun 6 '18 at 18:36
7
\$\begingroup\$

AngleRad class solves your problem, by displaying radians as degrees in debugger. But what problem is solved by AngleDeg class? I don't see any benifits:

  • It makes things more complicated, because now you have two similar classes with very similar names. Easy to confuse them.
  • Implicit conversion from double amplifies this confusion. AngleDeg angle = Math.Atan(...) - this is now a valid code. I strongly recommend against such implicit conversion.
  • Converting back and forth between two representations slowly builds up a round-off error, that otherwise can be avoided (by making all your calculations using radians).

So bottom line is: I would drop the first class and keep the second one.

P.S. How it would affect performance is something you have to measure on your own. Run a profiler, or write a benchmark that models your calculations. The cost of implicit casting is negligible in most cases, but it can be noticeable if your app, say, multiplies angles 24/7. In that case this cost quickly adds up.

\$\endgroup\$
  • 1
    \$\begingroup\$ Agree with implicit conversion. Would suggest there be static methods named FromDegrees and FromRadians, though FromRadians is same as doing new instance. This would be similar to a TimeSpan.FromMinutes. \$\endgroup\$ – Rick Davin Jun 6 '18 at 17:44
2
\$\begingroup\$

I agree with other comments on the two classes have overlap.

You perform conversion in the ToString() and ToDegrees(). Reference the ToDegrees(). Actually I would just make Degrees a property.

I think one struct Angle with properties Radians and Degrees would be better.

\$\endgroup\$
2
\$\begingroup\$

The question is: What do you think of the possible drawbacks, pitfalls if we used these structs in a performance critical application? 

About your ToString methods:

Interpolated strings ($"{variable}...") are the antithesis of performance -- potentially worse than using StringBuilder for a similar construct. Each time the string is interpolated at runtime: a new compile time generated class is instantiated; a closure is created; the operations you expect are performed; and the closure and class instance are cleaned up. If your displaying a single value, this is negligible. If your displaying 1,000s of values (such as for UI display or reporting), this will begin take a hit on the CPU and memory pool. Consider just concatenating the double.ToString output with the degrees symbol.

The default ToString method makes assumptions about the display format. Consider adding an overload taking a string format parameter and passing it to the double.ToString call.

\$\endgroup\$
  • 1
    \$\begingroup\$ You mentioned performance and interpolated strings ($"{variable}...") are the antithesis of performance where did you get this from? OP doesn't mention that :-| \$\endgroup\$ – t3chb0t Jun 7 '18 at 6:19
  • \$\begingroup\$ I think OP is concerned with the performance of actual calculations. ToString method takes no part in those and is unlikely to become a bottleneck. So, personally, in this case I would go for "prettier" code. \$\endgroup\$ – Nikita B Jun 7 '18 at 7:29
  • 1
    \$\begingroup\$ Why do you think that Interpolated strings ($"{variable}...") are the antithesis of performance -- potentially worse than using StringBuilder for a similar construct - do you have any proof for that? They are just a syntactic sugar. The compiler will generate the same code as if you used string.Format and if you are not doing this in a loop there will hardly be any difference noticable. \$\endgroup\$ – t3chb0t Jun 7 '18 at 15:43
  • 3
    \$\begingroup\$ @t3chb0t Upon re-reading the documentation, I had overthought the FormattableString and IFormattable implicit conversions. My understanding was that an IFormattable implementation was always generated and run. I have confirmed your statements and am striking my performance claims. \$\endgroup\$ – psaxton Jun 7 '18 at 17:14
1
\$\begingroup\$

UPDATE

I've gotten several downvotes with no comments for improvement. I think I gave sound advice that it is overkill for LayoutKind.Sequential and AggressiveInlining. I think it's good advice to have a Normalize method - as requested by the OP - which is a better name than ClampInRangeOf0To360Degrees. I also think its good advice that the ToString methods could be more flexible to allow for a varying number of decimal digits. If I am wrong on these points, please educate me.

I am going to assume the silent downvotes were from the code. I have modified the code. The previous constant factors have been made static methods. I trust readers understand that my code is offered as an example of possibilities the OP can explore.

I have nothing to add to Henrik Hansen and Nikita B's answers. I agree with them. I think you have way too much overkill and premature optimization with InteropServices.LayoutKind.Sequential or AggressiveInlining. No reason for LayoutKind if there is really only 1 value object in the struct.

String methods will be slower than purely numeric methods. I find your structs rigid in that they fix the decimals at 2.

I offer an alternative to demonstrate Henrik's and Nikita's answers:

public struct Angle
{
    // You don't have to make this private.  
    // It's offered merely as an alternative to always
    // force someone to use FromRadians or FromDegrees.
    private Angle(double radians)
    {
        Radians = Normalize(radians);
    }

    public double Radians { get; }
    public double Degrees => RadiansToDegrees(Radians);
    public double Gradians => RadiansToGradians(Radians);

    private static double Normalize(double radians) => radians % TwoPI;

    public const double TwoPI = 2 * Math.PI;
    public static double RadiansToDegrees(double radians) => radians * 180 / Math.PI;
    public static double RadiansToGradians(double radians) => radians * 200 / Math.PI;
    public static double DegreesToRadians(double degrees) => degrees * Math.PI / 180;
    public static double GradiansToRadians(double gradians) => gradians * Math.PI / 200;

    public static Angle FromRadians(double radians) => new Angle(radians);
    public static Angle FromDegrees(double degrees) => new Angle(DegreesToRadians(degrees));
    public static Angle FromGradians(double gradians) => new Angle(GradiansToRadians(gradians));

    public override string ToString() => RadiansAsString();
    public string ToString(int decimals) => RadiansAsString(decimals);
    public string RadiansAsString(int decimals = 2) => $"{Radians.ToString($"F{decimals}")} rad";
    public string DegreesAsString(int decimals = 2) => $"{Degrees.ToString($"F{decimals}")}°";
    public string GradiansAsString(int decimals = 2) => $"{Gradians.ToString($"F{decimals}")} g";
}

You only need 1 struct and it should have a simple name: Angle. I have constants defined to replace the need for aggressive inlining. Since you mentioned the need to normalize between 0-360 degrees, I have a Normalize method.

I intentionally use a private constructor so that I force others to use FromRadians, FromDegrees, or even FromGradians. The private constructor is purely optional. The implicit operators are gone. They really can lead to hard-to-find bugs since what double did you want to implicitly convert: radians or degrees? Your code will be more understandable by clearly stating the purpose that its FromDegrees versus FromRadians.

To put it another way, let's say I have a variable named "myDegrees" despite the fact that I hate prefacing variable named with "my". This code would be easy to gloss over:

var angle = new Angle(myDegrees);  // if constructor was public

But this code should catch your eye that there is an issue:

var angle = Angle.FromRadians(myDegrees);

Because there is clear conflict with the names.

My string methods allow for a varying number of decimal digits to be displayed.

There is no error checking and stuff like that in my code.

If you only input and output with degrees

Then you may still have one structure where all internal calculations are based on radians but the inputs and ToString outputs are in your preference of degrees. This also means you could keep an implicit cast from double degrees to Angle. That code could look something like:

public struct Angle
{
    // Prefer input as degrees
    private Angle(double degrees)
    {
        Radians = Normalize(DegreesToRadians(degrees));
    }

    public double Radians { get; }
    public double Degrees => RadiansToDegrees(Radians);
    public double Gradians => RadiansToGradians(Radians);

    private static double Normalize(double radians) => radians % TwoPI;

    public const double TwoPI = 2 * Math.PI;
    public static double RadiansToDegrees(double radians) => radians * 180 / Math.PI;
    public static double RadiansToGradians(double radians) => radians * 200 / Math.PI;
    public static double DegreesToRadians(double degrees) => degrees * Math.PI / 180;
    public static double GradiansToRadians(double gradians) => gradians * Math.PI / 200;

    public static Angle FromRadians(double radians) => new Angle(radians);
    public static Angle FromDegrees(double degrees) => new Angle(DegreesToRadians(degrees));
    public static Angle FromGradians(double gradians) => new Angle(GradiansToRadians(gradians));

    // Prefer output as degrees
    public override string ToString() => DegreesAsString();
    public string ToString(int decimals) => DegreesAsString(decimals);

    public string RadiansAsString(int decimals = 2) => $"{Radians.ToString($"F{decimals}")} rad";
    public string DegreesAsString(int decimals = 2) => $"{Degrees.ToString($"F{decimals}")}°";
    public string GradiansAsString(int decimals = 2) => $"{Gradians.ToString($"F{decimals}")} g";

    // Prefer implicit input of degrees
    public static implicit operator Angle(double degrees) => Angle.FromDegrees(degrees);

    // but internal operations are on radians
    public static Angle operator +(Angle a, Angle b) => Angle.FromRadians(a.Radians + b.Radians);
    public static Angle operator -(Angle a, Angle b) => Angle.FromRadians(a.Radians - b.Radians);
    public static Angle operator *(Angle a, Angle b) => Angle.FromRadians(a.Radians * b.Radians);
    public static Angle operator /(Angle a, Angle b) => Angle.FromRadians(a.Radians / b.Radians);
}

The math operators offer the benefit of creating a new, immutable Angle, which also means the Normalize method is applied with each math operation.

\$\endgroup\$
  • \$\begingroup\$ How come the Radians property is private? \$\endgroup\$ – Pod Jun 8 '18 at 9:14
  • 1
    \$\begingroup\$ @Pod. That would be a typo. Fixed. Thx. \$\endgroup\$ – Rick Davin Jun 8 '18 at 11:13
  • \$\begingroup\$ I'd written a comment but it was removed... and at that time I didn't vote at all so the first DV wasn't mine (but the second one is). I said that I didn't like calling the code an alternative which it isn't because it just implements suggestions from two different answers. If you could make it clear which part of the review is new and which one just shows an example implementation of the already mentioned improvements I'll gladly upvote the answer. \$\endgroup\$ – t3chb0t Jun 9 '18 at 9:40
  • \$\begingroup\$ @t3chb0t Thanks for feedback. The mention of it being an alternative was in reference to the OP's code, not to other answers. I disagree that it "just implements" two other answers, since it also implements points that I was first to mention. Those are summarized in the updated first paragraph. \$\endgroup\$ – Rick Davin Jun 9 '18 at 12:48

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.