8
\$\begingroup\$

I am working on a Units library for a personal project that is a physics-heavy C#/Unity3D game, and I am looking to improve it in a few areas:

  1. Is there a way to reduce the code duplication?

  2. Is performance going to be a problem? Is there anything I can do to improve performance?

  3. Can I hide the value member variable so it is not visible to the rest of the program, but only within this namespace?

  4. Are the Equals() and GetHashCode() methods implemented correctly?

  5. Are there other changes that I should do to make this more idiomatic C#?

[Serializable]
public struct Meter
{
    [SerializeField]
    internal double value;

    public Meter(double value) => this.value = value;

    public static Meter operator +(Meter m) => m;
    public static Meter operator -(Meter m) => new Meter() { value = -m.value };
    public static Meter operator +(Meter m1, Meter m2) => new Meter() { value = m1.value + m2.value };
    public static Meter operator -(Meter m1, Meter m2) => new Meter() { value = m1.value - m2.value };
    public static Meter operator *(Meter m, double d) => new Meter() { value = m.value * d };
    public static Meter operator *(double d, Meter m) => new Meter() { value = m.value * d };
    public static Meter operator /(Meter m, double d) => new Meter() { value = m.value / d };
    public static double operator /(Meter m1, Meter m2) => m1.value / m2.value;

    public static MeterSquared operator *(Meter m1, Meter m2) => new MeterSquared() { value = m1.value * m2.value };
    public static MeterPerSecond operator /(Meter m, Second s) => new MeterPerSecond() { value = m.value / s.value };

    public readonly static Meter zero = new Meter() { value = 0.0 };

    public override bool Equals(object obj) => obj is Meter other ? value.Equals(other.value) : false;
    public override int GetHashCode() => value.GetHashCode();
    public static bool operator ==(Meter m1, Meter m2) => m1.value == m2.value;
    public static bool operator !=(Meter m1, Meter m2) => m1.value != m2.value;
}

[Serializable]
public struct Second
{
    [SerializeField]
    internal double value;

    public Second(double value) => this.value = value;

    public static Second operator +(Second s) => s;
    public static Second operator -(Second s) => new Second() { value = -s.value };
    public static Second operator +(Second s1, Second s2) => new Second() { value = s1.value + s2.value };
    public static Second operator -(Second s1, Second s2) => new Second() { value = s1.value - s2.value };
    public static Second operator *(Second s, double d) => new Second() { value = s.value * d };
    public static Second operator /(Second s, double d) => new Second() { value = s.value / d };
    public static double operator /(Second s1, Second s2) => s1.value / s2.value;

    public readonly static Second zero = new Second() { value = 0.0 };

    public override bool Equals(object obj) => obj is Second other ? value.Equals(other.value) : false;
    public override int GetHashCode() => value.GetHashCode();
    public static bool operator ==(Second s1, Second s2) => s1.value == s2.value;
    public static bool operator !=(Second s1, Second s2) => s1.value != s2.value;
}


/// <summary>
/// m / s, velocity
/// </summary>
[Serializable]
public struct MeterPerSecond
{
    [SerializeField]
    internal double value;

    public MeterPerSecond(double value) => this.value = value;

    public static MeterPerSecond operator +(MeterPerSecond m) => m;
    public static MeterPerSecond operator -(MeterPerSecond m) => new MeterPerSecond() { value = -m.value };
    public static MeterPerSecond operator +(MeterPerSecond m1, MeterPerSecond m2) => new MeterPerSecond() { value = m1.value + m2.value };
    public static MeterPerSecond operator -(MeterPerSecond m1, MeterPerSecond m2) => new MeterPerSecond() { value = m1.value - m2.value };
    public static MeterPerSecond operator *(MeterPerSecond m, double d) => new MeterPerSecond() { value = m.value * d };
    public static MeterPerSecond operator /(MeterPerSecond m, double d) => new MeterPerSecond() { value = m.value / d };

    public static Meter operator *(MeterPerSecond m, Second s) => new Meter() { value = m.value * s.value };
    public static MeterPerSecondSquared operator /(MeterPerSecond m, Second s) => new MeterPerSecondSquared() { value = m.value * s.value };

    public readonly static MeterPerSecond zero = new MeterPerSecond() { value = 0.0 };

    public override bool Equals(object obj) => obj is MeterPerSecond other ? value.Equals(other.value) : false;
    public override int GetHashCode() => value.GetHashCode();
    public static bool operator ==(MeterPerSecond m1, MeterPerSecond m2) => m1.value == m2.value;
    public static bool operator !=(MeterPerSecond m1, MeterPerSecond m2) => m1.value != m2.value;
}

Example usage:

public class AstronomicalObject2 : MonoBehaviour
{
    public string astronomicalName;
    public Meter radius;

    [Header("Provide One")]
    public Kilogram mass;
    public MeterPerSecondSquared surfaceGravity;
    public KilogramPerCubicMeter meanDensity;

    [Header("Calculated")]
    public Meter circumference;
    public MeterSquared surfaceArea;
    public MeterCubed volume;
    public StandardGravitationParameter gm;

    [Header("Atmo-Required")]
    public Pascal atmosphereSurfacePressure = new Pascal(101325);
    public Meter atmosphereScaleHeight = new Meter(8500);

    [Header("Atmo-Calculated")]
    public Meter atmosphereMaxHeight;

    public readonly Pascal atmospherePressureCutoff = new Pascal(1);

    private Kilogram old_mass;
    private MeterPerSecondSquared old_surfaceGravity;
    private KilogramPerCubicMeter old_meanDensity;

    void OnEnable()
    {
        old_mass = mass;
        old_surfaceGravity = surfaceGravity;
        old_meanDensity = meanDensity;
    }

    void OnValidate()
    {
        circumference = 2.0 * Math.PI * radius;
        surfaceArea = circumference * 2.0 * radius; // 4.0 * Math.PI * radius * radius;
        volume = surfaceArea * radius / 3.0; // 4.0 / 3.0 * Math.PI * radius * radius * radius;

        if (mass != old_mass)
        {
            gm = GravitationConstant.G * mass;
            surfaceGravity = gm / (radius * radius);
            meanDensity = mass / volume;
        }
        else if (surfaceGravity != old_surfaceGravity)
        {
            mass = new Kilogram(surfaceGravity.value * radius.value * radius.value / GravitationConstant.G.value);
            gm = GravitationConstant.G * mass;
            meanDensity = mass / volume;
        }
        else if (meanDensity != old_meanDensity)
        {
            mass = meanDensity * volume;
            gm = GravitationConstant.G * mass;
            surfaceGravity = gm / (radius * radius);
        }
        else
        {
            // Something else changed
            // Make sure all the calculated values are accurate, assuming that the mass is the desired value
            gm = GravitationConstant.G * mass;
            surfaceGravity = gm / (radius * radius);
            meanDensity = mass / volume;
        }

        old_mass = mass;
        old_surfaceGravity = surfaceGravity;
        old_meanDensity = meanDensity;

        // Atmo
        if (atmosphereSurfacePressure.value > 0)
        {
            atmosphereMaxHeight = -atmosphereScaleHeight * Math.Log(atmospherePressureCutoff / atmosphereSurfacePressure);
        }
        else
        {
            atmosphereMaxHeight = Meter.zero;
        }
    }
}

Alternative without the Units classes:

// Excerpt from the OnValidate() method:
circumference_m = 2.0 * Math.PI * radius_m; // m
surfaceArea_m2 = circumference_m * 2.0 * radius_m; // 4.0 * Math.PI * radius * radius = m^2
volume_m3 = surfaceArea_m2 * radius_m / 3.0; // 4.0 / 3.0 * Math.PI * radius * radius * radius = m^3

if (mass_kg != old_mass)
{
    gm = PhysicsConstants.G * mass_kg; // (m^3/(kg*s^2)) * kg = m^3/s^2
    surfaceGravity_m_s = gm / (radius_m * radius_m); // (m^3/s^2) / m^2 = m/s^2
    meanDensity_kg_m3 = mass_kg / volume_m3; // kg / m^3 = kg/m^3
}
else if (surfaceGravity_m_s != old_surfaceGravity)
{
    mass_kg = surfaceGravity_m_s * radius_m * radius_m / PhysicsConstants.G; // (m/s^2 * m * m) / (m^3/(kg*s^2)) = kg
    gm = PhysicsConstants.G * mass_kg; // (m^3/(kg*s^2)) * kg = m^3/s^2
    meanDensity_kg_m3 = mass_kg / volume_m3; // kg / m^3 = kg/m^3
}
else if (meanDensity_kg_m3 != old_meanDensity)
{
    mass_kg = meanDensity_kg_m3 * volume_m3; // kg/m^3 * m^3 = kg
    gm = PhysicsConstants.G * mass_kg; // (m^3/(kg*s^2)) * kg = m^3/s^2
    surfaceGravity_m_s = gm / (radius_m * radius_m); // (m^3/s^2) / m^2 = m/s^2
}
else
{
    // Something else changed
    // Make sure all the calculated values are accurate, assuming that the mass is the desired value
    gm = PhysicsConstants.G * mass_kg; // (m^3/(kg*s^2)) * kg = m^3/s^2
    surfaceGravity_m_s = gm / (radius_m * radius_m); // (m^3/s^2) / m^2 = m/s^2
    meanDensity_kg_m3 = mass_kg / volume_m3; // kg / m^3 = kg/m^3
}

Thanks for looking at this!

Edit: Added the example usage, plus an example without the Units classes. The purpose of this library is to avoid units errors, like using velocity (m/s) when it should be length (m) or something like that. I am not sure if this is worth the extra effort though... On a work project in the past, I used a nice units library but it was in C++ and made extensive use of templates to make it easy to use and easy to maintain and performant. Not sure that I can achieve the same in C#.

\$\endgroup\$
9
  • \$\begingroup\$ would you have to have a new struct for every new unit? so for kilometres or milliseconds? \$\endgroup\$
    – Tim Rutter
    Jan 3, 2020 at 15:07
  • \$\begingroup\$ No, I was only thinking of having a type for the base unit, meters, and not one for each prefix (kilometer, millimeter, etc). I also do not think I will create a type for non-metric units (feet, etc). \$\endgroup\$
    – Sirius 5
    Jan 3, 2020 at 18:09
  • \$\begingroup\$ @Sirius5 whats the idea of those structs in the first place? Having fluent interfaces? Also, MeterSquared and MeterPerSecondSquared are not present. \$\endgroup\$
    – kuskmen
    Jan 3, 2020 at 20:40
  • \$\begingroup\$ Having fluent interfaces and to prevent errors due to using the wrong units in a calculation (like multiplying mass times length instead of length times length, or something). These three structs are just an example of the more than a dozen that I have implemented so far. \$\endgroup\$
    – Sirius 5
    Jan 4, 2020 at 2:04
  • \$\begingroup\$ One example of a calculation in my code is: mass = surfaceGravity * radius * radius / PhysicsConstants.G;, for which the units are: kg = m/s^2 * m * m / (m^3/(kg*s^2)). So I need definitions for kg, m/s^2, m, and m^3/(kg*s^2). But I also need m^2/s^2, because it first multiplies m/s^2 * m, then m^3/s^2 because it multiplies by another m. The list of unit types is becoming unwieldy, especially with how many lines of code I have to maintain for each struct. If the definitions were shorter, this might be more manageable. \$\endgroup\$
    – Sirius 5
    Jan 4, 2020 at 2:09

3 Answers 3

2
\$\begingroup\$

Can I hide the value member variable so it is not visible to the rest of the program, but only within this namespace?

I prefer to move independent modules to external dll library (libraries), in which case I may use internals magic and then expose only public classes to outer world. Unity3d's Assembly Definition system basically does the same thing.

In your case you could make value private and create public property:

[SerializeField]
private double value;
public double Value => value;

So now external scripts cannot modify internal state of the struct, which generally is desired behavior.

Are the Equals() and GetHashCode() methods implemented correctly?

Equals() can be simplified a little:

obj is Meter other && value.Equals(other.value);

GetHashCode(), on the other hand, references value field, which is mutable. Part of my answer already covered it, but if you'll keep your value internal, evil might happen:

var unit = new Unit(1);
var dictionary = new Dictionary<Unit, int> {[unit] = 1};
unit.value = 2;
Assert.True(dictionary.ContainsKey(unit)); // Is it true? Is it false?

You might think it's convenient that Unit with new value represents completely different entity which no longer exists inside the dictionary... Or you might think that Unit is still the same object whatever its value is. Or you might accidentally modify value and wonder why your dictionary returns KeyNotFoundException.

Are there other changes that I should do to make this more idiomatic C#?

There might be problem with equality operations. Consider following example:

double a = 0.1d;
double b = 0.2d;
double c = 0.3d;
Assert.True(a + b == c); // Actually it's false.

I prefer instead of m1.value == m2.value use Math.Abs(m1.value - m2.value) < tolerance.

\$\endgroup\$
1
\$\begingroup\$

Extending from my comment on the question, I'd probably go ahead and implement the less than and greater than operators and a handful of interfaces like the underlying double has. I'm also a big proponent of the struct itself being immutable. The result looks something like (I only did Meter for this exercise):

[Serializable]
public struct Meter : IComparable, IFormattable, IConvertible, IComparable<Meter>, IEquatable<Meter>
{
    public Meter(double value) => this.Value = value;

    public static Meter Zero { get; } = new Meter(0.0);

    internal double Value { get; }

    public static Meter operator +(Meter m) => m;

    public static Meter operator -(Meter m) => new Meter(-m.Value);

    public static Meter operator +(Meter m1, Meter m2) => new Meter(m1.Value + m2.Value);

    public static Meter operator -(Meter m1, Meter m2) => new Meter(m1.Value - m2.Value);

    public static Meter operator *(Meter m, double d) => new Meter(m.Value * d);

    public static Meter operator *(double d, Meter m) => new Meter(m.Value * d);

    public static Meter operator /(Meter m, double d) => new Meter(m.Value / d);

    public static double operator /(Meter m1, Meter m2) => m1.Value / m2.Value;

    public static MeterSquared operator *(Meter m1, Meter m2) => new MeterSquared(m1.Value * m2.Value);

    public static MeterPerSecond operator /(Meter m, Second s) => new MeterPerSecond(m.Value / s.Value);

    public static bool operator ==(Meter m1, Meter m2) => m1.Value == m2.Value;

    public static bool operator !=(Meter m1, Meter m2) => m1.Value != m2.Value;

    public static bool operator <(Meter m1, Meter m2) => m1.Value < m2.Value;

    public static bool operator <=(Meter m1, Meter m2) => m1.Value <= m2.Value;

    public static bool operator >(Meter m1, Meter m2) => m1.Value > m2.Value;

    public static bool operator >=(Meter m1, Meter m2) => m1.Value >= m2.Value;

    public override bool Equals(object obj) => obj is Meter other && this.Equals(other);

    public bool Equals(Meter other) => this.Value.Equals(other.Value);

    public override int GetHashCode()
    {
        unchecked
        {
            return 17 * this.Value.GetHashCode();
        }
    }

    public string ToString(string format, IFormatProvider formatProvider) => this.Value.ToString(format, formatProvider) + "m";

    public override string ToString() => this.ToString(null, CultureInfo.CurrentCulture);

    public string ToString(IFormatProvider provider) => this.ToString(null, provider);

    public int CompareTo(object obj)
    {
        if (obj is null)
        {
            return 1;
        }

        if (!(obj is Meter m))
        {
            throw new ArgumentException("Can only compare with another Meter.");
        }

        return this.CompareTo(m);
    }

    public int CompareTo(Meter other)
    {
        if (this < other)
        {
            return -1;
        }

        if (this > other)
        {
            return 1;
        }

        return 0;
    }

    public TypeCode GetTypeCode() => TypeCode.Double;

    public bool ToBoolean(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Boolean)}");

    public char ToChar(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Char)}");

    public sbyte ToSByte(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(SByte)}");

    public byte ToByte(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Byte)}");

    public short ToInt16(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Int16)}");

    public ushort ToUInt16(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(UInt16)}");

    public int ToInt32(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Int32)}");

    public uint ToUInt32(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(UInt32)}");

    public long ToInt64(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Int64)}");

    public ulong ToUInt64(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(UInt64)}");

    public float ToSingle(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Single)}");

    public double ToDouble(IFormatProvider provider) => this.Value;

    public decimal ToDecimal(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(Decimal)}");

    public DateTime ToDateTime(IFormatProvider provider) => throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {nameof(DateTime)}");

    public object ToType(Type conversionType, IFormatProvider provider)
    {
        if (conversionType is null)
        {
            throw new ArgumentNullException(nameof(conversionType));
        }

        if (conversionType == typeof(Meter))
        {
            return this;
        }

        if (conversionType == typeof(double))
        {
            return this.Value;
        }

        if (conversionType == typeof(string))
        {
            return this.ToString(provider);
        }

        throw new InvalidCastException($"Cannot cast from {nameof(Meter)} to {conversionType.Name}");
    }
}
\$\endgroup\$
0
\$\begingroup\$

Now, two years later you could simplify this code by using the new record struct. It'll take care of all the tricky equality comparisons.


There are lines in your code lines where you use ordinary types like double:

public static Meter operator *(Meter m, double d) => new Meter() { value = m.value * d };

I wouldn't do it because there is a risk of mixing units. If let's say, there's some other type that implicitly can convert to double or some other number, you might accidentally do some illegal math.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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