Class for handling unit conversions

Question

I am creating a class to be the end all defacto class for handling unit conversions. This is particularly difficult in my company's industry where imperial architectural strings are used as units. I wanted this class to be perfect in just about every way as it is the class that we will be giving our summer interns as the best example of how to write a class. By posting it here, I wanted to make sure that it is the best example!

The references to the DimensionConverter class is to a private class built just for this purpose. (I would like it reviewed as well... but I don't know how to post two classes appropriately here on Code Review.)

What else could I possibly improve about the way this class is written in the context of readability, ease of maintenance, and logical design?

namespace TypeLibrary
{
 /// <summary>
 /// Class used for storing dimensions that may need to be accessed in a different measurement system
 /// Will accept anything as input
 /// <example>
 /// decimal inches into AutoCAD notation
 /// 
 /// double inches = 14.1875
 /// Dimension dm = new Dimension( inches, DimensionTypes.Inch);
 /// 
 /// Print(dm.Architectural)
 /// 
 /// ========Output==========
 /// 1'2 3/16"
 /// 
 /// </example>
 /// </summary>
 public class Dimension
 {
 #region internal variables
 //internal Dimension type is set to milimeter to cause the least amount of rounding error when performing calculations.
 const DimensionTypes internalUnitType = DimensionTypes.Millimeter;
 //dimension value
 private double internalDecimalUnit = 0.0;
 #endregion
 #region Constructors
 /// <summary>
 /// Accepts any string value for input.
 /// </summary>
 public Dimension(string passedArchitecturalString)
 {
 storeArchitecturalStringAsInternalUnit(passedArchitecturalString);
 }
 /// <summary>
 /// Accepts any type of value for input. This allows for less complexity in building applications
 /// </summary>
 public Dimension(double passedInput, DimensionTypes passedDimensionType)
 {
 internalDecimalUnit = DimensionConverter.ConvertDimension(passedDimensionType, passedInput, internalUnitType);
 }
 /// <summary>
 /// Copy Constructor
 /// </summary>
 public Dimension(Dimension passedDimension)
 {
 this.internalDecimalUnit = passedDimension.internalDecimalUnit;
 }
 #endregion
 #region Properties
 public double Sixteenths
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Sixteenth); }
 set { storeAsInternalUnit(DimensionTypes.Sixteenth, value); }
 }
 public double ThirtySeconds
 {
 get { return retrieveAsExternalUnit(DimensionTypes.ThirtySecond); }
 set { storeAsInternalUnit(DimensionTypes.ThirtySecond, value); }
 }
 public double Inches
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Inch); }
 set { storeAsInternalUnit(DimensionTypes.Inch, value); }
 }
 public double Feet
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Foot); }
 set { storeAsInternalUnit(DimensionTypes.Foot, value); }
 }
 public double Yards
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Yard); }
 set { storeAsInternalUnit(DimensionTypes.Yard, value); }
 }
 public double Miles
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Mile); }
 set { storeAsInternalUnit(DimensionTypes.Mile, value); }
 }
 public double Millimeters
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Millimeter); }
 set { storeAsInternalUnit(DimensionTypes.Millimeter, value); }
 }
 public double Centimeters
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Centimeter); }
 set { storeAsInternalUnit(DimensionTypes.Centimeter, value); }
 }
 public double Meters
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Meter); }
 set { storeAsInternalUnit(DimensionTypes.Meter, value); }
 }
 public double Kilometers
 {
 get { return retrieveAsExternalUnit(DimensionTypes.Kilometer); }
 set { storeAsInternalUnit(DimensionTypes.Kilometer, value); }
 }
 /// <summary>
 /// Returns the dimension as a string in AutoCAD notation with sixteenths of an inch percision
 /// </summary>
 public string Architectural
 {
 get { return retrieveInternalUnitAsArchitecturalString(); }
 set {storeArchitecturalStringAsInternalUnit(value); }
 }
 #endregion
 #region helper methods
 private void storeAsInternalUnit(DimensionTypes fromDimensionType, double passedValue)
 {
 internalDecimalUnit = DimensionConverter.ConvertDimension( DimensionTypes.Kilometer, passedValue, internalUnitType);
 }
 private void storeArchitecturalStringAsInternalUnit(string passedArchitecturalString)
 {
 internalDecimalUnit = DimensionConverter.ConvertArchitecturalStringToDecimalDimension(internalUnitType, passedArchitecturalString);
 }
 private double retrieveAsExternalUnit(DimensionTypes toDimensionType)
 {
 return DimensionConverter.ConvertDimension(internalUnitType, internalDecimalUnit, toDimensionType);
 }
 private string retrieveInternalUnitAsArchitecturalString()
 {
 return DimensionConverter.ConvertDecimalDimensionToArchitecturalString(internalUnitType, internalDecimalUnit);
 }
 #endregion
 #region Overloaded Operators
 /* You may notice that we do not overload the increment and decrement operators.
 * This is because the user of this library
 * does not know what is being internally stored. We could have allowed a parameter
 * into a custom operater and method of our own, but that would have added unnecessary
 * complexity.
 */
 public static Dimension operator +(Dimension d1, Dimension d2)
 {
 //add the two dimensions together
 //return a new dimension with the new value
 return new Dimension((d1.internalDecimalUnit + d2.internalDecimalUnit), internalUnitType);
 }
 public static Dimension operator -(Dimension d1, Dimension d2)
 {
 //subtract the two dimensions
 //return a new dimension with the new value
 return new Dimension((d1.internalDecimalUnit - d2.internalDecimalUnit), internalUnitType);
 }
 public static Dimension operator *(Dimension d1, Dimension d2)
 {
 //multiply the two dimensions
 //return a new dimension with the new value
 return new Dimension((d1.internalDecimalUnit * d2.internalDecimalUnit), internalUnitType);
 }
 public static Dimension operator /(Dimension d1, Dimension d2)
 {
 //divide the two dimensions
 //return a new dimension with the new value
 return new Dimension((d1.internalDecimalUnit / d2.internalDecimalUnit), internalUnitType);
 }
 public static Dimension operator %(Dimension d1, Dimension d2)
 {
 //Modulous the two dimensions
 //return a new dimension with the new value
 return new Dimension((d1.internalDecimalUnit % d2.internalDecimalUnit), internalUnitType);
 }
 public static bool operator ==(Dimension d1, Dimension d2)
 {
 //compare the two dimensions for equality
 //return a bool based on their relative values
 if (d1.internalDecimalUnit == d2.internalDecimalUnit) 
 {
 return true;
 }
 else
 {
 return false;
 }
 }
 public static bool operator !=(Dimension d1, Dimension d2)
 {
 //compare the two dimensions for equality
 //return a bool based on their relative values
 if (d1.internalDecimalUnit != d2.internalDecimalUnit)
 {
 return true;
 }
 else
 {
 return false;
 }
 }
 public static bool operator >(Dimension d1, Dimension d2)
 {
 //compare the two dimensions together
 //return a bool based on their relative values
 if (d1.internalDecimalUnit > d2.internalDecimalUnit)
 {
 return true;
 }
 else
 {
 return false;
 }
 }
 public static bool operator <(Dimension d1, Dimension d2)
 {
 //compare the two dimensions together
 //return a bool based on their relative values
 if (d1.internalDecimalUnit < d2.internalDecimalUnit)
 {
 return true;
 }
 else
 {
 return false;
 }
 }
 /// <summary>
 /// This override determines how this object is inserted into hashtables.
 /// </summary>
 /// <returns>same hashcode as any double would</returns>
 public override int GetHashCode()
 {
 return internalDecimalUnit.GetHashCode();
 }
 public override string ToString()
 {
 throw new NotImplementedException("The Dimension class does not know what type of unit it contains, (Because it should be thought of containing all unit types) Call dimension.[unit].Tostring() instead");
 }
 #endregion
 }
}

The second class that does the real work

namespace TypeLibrary
{
 /// <summary> 
 /// Contains all dimension conversion tools
 /// </summary>
 internal static class DimensionConverter
 {
 #region private functions
 /// <summary>
 /// Converts any possible type of Architectual String into Millimeters
 /// <remarks>Throws FormatException on bad input</remarks>
 /// </summary>
 /// <param name="arch">the input string</param>
 /// <returns>decimal Millimeters</returns>
 private static Double ConvertArchitectualStringtoMillimeters(String arch)
 {
 // for details on where this solution came from, check here: http://stackoverflow.com/questions/22794466/parsing-all-possible-types-of-varying-architectural-dimension-input
 // answer by Trygve Flathen: http://stackoverflow.com/users/2795177/trygve-flathen
 String expr = "^\\s*(?<minus>-)?\\s*(((?<feet>\\d+)(?<inch>\\d{2})(?<sixt>\\d{2}))|((?<feet>[\\d.]+)')?[\\s-]*((?<inch>\\d+)?[\\s-]*((?<numer>\\d+)/(?<denom>\\d+))?\")?)\\s*$";
 Match m = new Regex(expr).Match(arch);
 if (!m.Success || arch.Trim() == "" || arch.Trim() == "\"")
 {
 FormatException exception = new FormatException("Input was not a valid architectural string");
 ExceptionHandler.ProcessException(exception);
 throw exception;
 }
 Int32 sign = m.Groups["minus"].Success ? -1 : 1;
 Double feet = m.Groups["feet"].Success ? Convert.ToDouble(m.Groups["feet"].Value) : 0;
 Int32 inch = m.Groups["inch"].Success ? Convert.ToInt32(m.Groups["inch"].Value) : 0;
 Int32 sixt = m.Groups["sixt"].Success ? Convert.ToInt32(m.Groups["sixt"].Value) : 0;
 Int32 numer = m.Groups["numer"].Success ? Convert.ToInt32(m.Groups["numer"].Value) : 0;
 Int32 denom = m.Groups["denom"].Success ? Convert.ToInt32(m.Groups["denom"].Value) : 1;
 double result = sign * (feet * 12 + inch + sixt / 16.0 + numer / Convert.ToDouble(denom));
 return ConvertDimension(DimensionType.Millimeter, result, DimensionType.Inch);
 }
 /// <summary>
 /// Returns a string formatted in a standard AutoCAD format
 /// </summary>
 /// <param name="num"> the number being converted into a string</param>
 /// <returns></returns>
 private static string ConvertMillimetersToArchitecturalString(double num, int precision = 16)
 {
 //Convert into inches before proceeding
 num = ConvertDimension(DimensionType.Inch, num, DimensionType.Millimeter);
 string functionReturnValue = "";
 int feet = 0;
 int inches = 0;
 string FeetString = "";
 string InchesString = "";
 int numerator = 0;
 string sign = "";
 string fractionString = "";
 //detect need for sign
 if (num < 0)
 {
 sign = "-";
 num = num * -1;
 }
 feet = Convert.ToInt32(num / 12);
 num = num - (feet * 12);
 inches = Convert.ToInt32(num);
 num = num - inches;
 numerator = Convert.ToInt32(num * precision);
 if (numerator >= 32)
 {
 numerator = numerator - precision;
 inches = inches + 1;
 }
 fractionString = numerator + "/" + precision.ToString() + "\"";
 if (feet == 0)
 {
 FeetString = "";
 }
 else
 {
 FeetString = Convert.ToString(feet);
 }
 if (inches == 0)
 {
 InchesString = "";
 }
 else
 {
 InchesString = Convert.ToString(inches);
 }
 if (string.IsNullOrEmpty(sign) & string.IsNullOrEmpty(FeetString))
 {
 functionReturnValue = (InchesString + " " + fractionString).Trim();
 }
 else
 {
 functionReturnValue = (sign + FeetString + "' " + InchesString + " " + fractionString).Trim();
 }
 return functionReturnValue;
 }
 #endregion
 #region public functions
 /// <summary>
 /// Converts any dimension into another
 /// </summary>
 /// <param name="typeConvertingTo">input unit type</param>
 /// <param name="typeConvertingFrom">desired output unit type</param>
 /// <returns>converted dimension</returns>
 public static double ConvertDimension(DimensionType typeConvertingFrom, double passedValue, DimensionType typeConvertingTo)
 {
 double returnDouble = 0.0;
 double internalDecimalMillimeters = 0.0;
 //first convert value passedValue to inches
 switch (typeConvertingFrom)
 {
 case DimensionType.ThirtySecond:
 internalDecimalMillimeters = (passedValue / 32) * 0.0393701;
 break;
 case DimensionType.Sixteenth:
 internalDecimalMillimeters = (passedValue / 16) * 0.0393701;
 break;
 case DimensionType.Inch:
 internalDecimalMillimeters = passedValue * 25.4;
 break;
 case DimensionType.Foot:
 internalDecimalMillimeters = passedValue * 304.8;
 break;
 case DimensionType.Yard:
 internalDecimalMillimeters = passedValue * 914.4;
 break;
 case DimensionType.Mile:
 internalDecimalMillimeters = passedValue * 1609344;
 break;
 case DimensionType.Millimeter:
 internalDecimalMillimeters = passedValue;
 break;
 case DimensionType.Centimeter:
 internalDecimalMillimeters = passedValue * 10;
 break;
 case DimensionType.Meter:
 internalDecimalMillimeters = passedValue * 1000;
 break;
 case DimensionType.Kilometer:
 internalDecimalMillimeters = passedValue * 1000000;
 break;
 }
 //Now convert the value from inches to the desired output
 switch (typeConvertingTo)
 {
 case DimensionType.ThirtySecond:
 returnDouble = (internalDecimalMillimeters / 0.0393701) * 32;
 break;
 case DimensionType.Sixteenth:
 returnDouble = (internalDecimalMillimeters / 0.0393701) * 16;
 break;
 case DimensionType.Inch:
 returnDouble = internalDecimalMillimeters / 25.4;
 break;
 case DimensionType.Foot:
 returnDouble = internalDecimalMillimeters / 304.8;
 break;
 case DimensionType.Yard:
 returnDouble = internalDecimalMillimeters / 914.4;
 break;
 case DimensionType.Mile:
 returnDouble = internalDecimalMillimeters / 1609344;
 break;
 case DimensionType.Millimeter:
 returnDouble = internalDecimalMillimeters;
 break;
 case DimensionType.Centimeter:
 returnDouble = internalDecimalMillimeters / 10;
 break;
 case DimensionType.Meter:
 returnDouble = internalDecimalMillimeters / 1000;
 break;
 case DimensionType.Kilometer:
 returnDouble = internalDecimalMillimeters / 1000000;
 break;
 }
 return returnDouble;
 }
 /// <summary>
 /// Converts Architectural strings into a decimal unit
 /// </summary>
 public static double ConvertArchitecturalStringToDecimalDimension(DimensionType typeConvertingTo, string passedValue)
 {
 double internalDecimalMillimeters = 0.0;
 //first convert value passedValue to millimeters
 internalDecimalMillimeters = ConvertArchitectualStringtoMillimeters(passedValue);
 //Now convert the value from millimeters to the desired output
 double result = ConvertDimension(DimensionType.Millimeter, internalDecimalMillimeters, typeConvertingTo);
 return result;
 }
 /// <summary>
 /// Converts any dimension into an architectural string representation
 /// </summary>
 /// <param name="typeConvertingFrom">desired output unit type</param>
 /// <returns>converted dimension</returns>
 public static string ConvertDecimalDimensionToArchitecturalString(DimensionType typeConvertingFrom, double passedValue)
 {
 double internalDecimalMillimeters = 0.0;
 //first convert value passedValue to millimeters
 ConvertDimension(DimensionType.Millimeter, passedValue, typeConvertingFrom);
 //Now convert the value from inches to the desired output
 return ConvertMillimetersToArchitecturalString(internalDecimalMillimeters);
 }
 #endregion
 }
}

EDIT

I have made some of the suggested changes from below. And I have written my UnitTests and come up with a few more questions.

Some people have suggested that helper classes like my DimensionConverter class are evil. should I combine the utility class with the dimension class and if so how?

Also what improvements should I make to my tests if I am assuming everything in DimensionConverter will be private to only the Dimension class?

UnitTests for the Dimension Class:

[TestClass]
public class DimensionTests
{
 /// <summary>
 /// Tests the architectural string constructor and the regular dimension constructor
 /// </summary>
 [TestMethod]
 public void Dimensions_Constructors()
 {
 // arrange & act
 //numeric value constructor
 Dimension inchDimension = new Dimension(DimensionType.Inch, 14.1875);
 //architectural string constructor
 Dimension architecturalDimension = new Dimension("1' 2 3/16\"");
 //copy constructor
 Dimension copiedDimension = new Dimension(architecturalDimension);
 // assert
 inchDimension.Millimeters.Should().Be(architecturalDimension.Millimeters);
 copiedDimension.ShouldBeEquivalentTo(architecturalDimension);
 }
 /// <summary>
 /// Tests mathmatical operators we will test the properties at the same time.
 /// </summary>
 [TestMethod]
 public void Dimensions_Math_Operators()
 {
 // arrange
 Dimension inchDimension = new Dimension(DimensionType.Inch, 14.1875);
 Dimension architecturalDimension = new Dimension("1'2 3/16\"");
 // act
 Dimension subtractionDimension = inchDimension - architecturalDimension;
 Dimension additionDimension = inchDimension + architecturalDimension;
 // assert
 subtractionDimension.Feet.Should().BeApproximately(0, .00000001, "Doubles math should get us at least this close");
 additionDimension.Millimeters.Should().BeApproximately(720.725, .00000001, "Doubles math should get us at least this close");
 additionDimension.Architectural.ShouldBeEquivalentTo("2'4 6/16\"");
 }
 /// <summary>
 /// Tests Architectural string inputs.
 /// </summary>
 [TestMethod]
 public void Dimensions_Architectural_Constructor()
 {
 // arrange
 Dimension dimension1 = new Dimension("1'2 3/16\"");
 Dimension dimension2 = new Dimension("1'");
 Dimension dimension3 = new Dimension("1'2\"");
 Dimension dimension4 = new Dimension("2 3/16\"");
 Dimension dimension5 = new Dimension("1'2-3/16\"");
 Dimension dimension6 = new Dimension("3/16\"");
 Dimension dimension7 = new Dimension("121103");
 Dimension dimension8 = new Dimension("-1'2\"");
 // assert
 dimension1.Architectural.ShouldBeEquivalentTo("1'2 3/16\"");
 dimension2.Architectural.ShouldBeEquivalentTo("1'");
 dimension3.Architectural.ShouldBeEquivalentTo("1'2\"");
 dimension4.Architectural.ShouldBeEquivalentTo("2 3/16\"");
 dimension5.Architectural.ShouldBeEquivalentTo("1'2 3/16\"");
 dimension6.Architectural.ShouldBeEquivalentTo("3/16\"");
 dimension7.Architectural.ShouldBeEquivalentTo("12'11 3/16\"");
 dimension8.Architectural.ShouldBeEquivalentTo("-1'2\"");
 }
 /// <summary>
 /// Tests all equality operators
 /// </summary>
 [TestMethod]
 public void Dimensions_Equality_Operators()
 {
 // arrange
 Dimension biggerDimension = new Dimension(DimensionType.Inch, 14.1875);
 Dimension smallerDimension = new Dimension("1' 2 1/16\"");
 Dimension equivalentbiggerDimension = new Dimension(DimensionType.Millimeter, 360.3625);
 // assert
 (smallerDimension < biggerDimension).Should().Be(true);
 (biggerDimension < smallerDimension).Should().Be(false);
 (biggerDimension > smallerDimension).Should().Be(true);
 (smallerDimension > biggerDimension).Should().Be(false);
 (equivalentbiggerDimension == biggerDimension).Should().Be(true);
 (equivalentbiggerDimension == smallerDimension).Should().Be(false);
 (equivalentbiggerDimension != smallerDimension).Should().Be(true);
 (equivalentbiggerDimension != biggerDimension).Should().Be(false);
 }
 /// <summary>
 /// Tests GetHashCodeOperation
 /// </summary>
 [TestMethod]
 public void Dimensions_GetHashCode()
 {
 // arrange
 Dimension dimension = new Dimension(DimensionType.Millimeter, 14.1875);
 double number = 14.1875;
 // act
 int dimensionHashCode = dimension.GetHashCode();
 int hashCode = number.GetHashCode();
 // assert
 hashCode.ShouldBeEquivalentTo(dimensionHashCode);
 }
 /// <summary>
 /// Tests toString failure
 /// </summary>
 [TestMethod]
 [ExpectedException(typeof(NotImplementedException))]
 public void Dimensions_ToString()
 {
 // arrange
 Dimension dimension = new Dimension(DimensionType.Millimeter, 14.1875);
 // act
 dimension.ToString();
 }
 /// <summary>
 /// Tests CompareTo implementation
 /// </summary>
 [TestMethod]
 public void Dimensions_CompareTo()
 {
 // arrange
 Dimension smallDimension = new Dimension(DimensionType.Millimeter, 1);
 Dimension mediumDimension = new Dimension(DimensionType.Foot, 1);
 Dimension largeDimension = new Dimension(DimensionType.Kilometer, 1);
 List<Dimension> dimensions = new List<Dimension>();
 dimensions.Add(smallDimension);
 dimensions.Add(largeDimension);
 dimensions.Add(mediumDimension);
 // act
 dimensions.Sort();
 // assert
 dimensions[0].ShouldBeEquivalentTo(smallDimension);
 dimensions[1].ShouldBeEquivalentTo(mediumDimension);
 dimensions[2].ShouldBeEquivalentTo(largeDimension);
 }
}

• svick: I think that unit conversions are not what your interns are going to write every day, so I'm not sure it's a good example. For example, how many classes overload arithmetic operators?
• jboy12: Strangely yes they will be writing a good many conversion tools where they will be converting output from one program to the desired input of others
• svick: If you want to post two classes, just post them.
• jboy12: Ok I will edit it in. Should I change my code as I fix the errors or leave it as is for future viewers?
• svick: No, you shouldn't, because that would invalidate existing answers. For more details, see this meta discussion.

Answer 1

Verbosity
Verbosity can be a good thing in code. The method name explains what the method does. Too much verbosity, though, can make your code tiring to read, as well as less memorable. For example - passedArchitecturalString - why call it passed? we know it is passed, it is an argument. Same for String - we know it is a string. Simply calling architectural would have conveyed the same information, and be a lot more memorable. internalDecimalUnit - again, we know it is internal, and it is actually not Decimal (it is double), I believe value would have been enough here, perhaps intrinsicValue if you want to be explicit.

Commentary
Comments have had their day when they were considered crucial to code readability. Nowadays, though, they have lost their shine a bit, and sometimes considered as a code smell. The main problem with comments, is that they tend to be forgotten as the code progresses, and often start making no sense, as they are not synchronized with the code. Another problem with them is that most programmers skip comments, and never actually read them.
Some of your comments are good (like for internalUnitType), but some are absolutely redundant (like for the copy constructor, or for the architectural constructor), and some just make the code harder to read rather than easier to read like the comments inside your operator overloadings.

Code consistency
It is a matter of style whether you want to address your internal members with the this. prefix, but once you make the choice, be consistent.

When helpers don't help
Your helper methods are one-liner delegations. This seems a bit redundant, since they don't reduce code complexity, nor enhance readability. They may even introduce bugs (let's see if you can find the bug in storeAsInternalUnit)

It always converts to Kilometers

Also, some helpers have only a single use, which makes them even more a waste of space. I would think that, for example, the following constructor code is more readable than yours, and saves you a method:

public Dimension(string architectural)
{
 Architectural = architectural;
}

The Immutables
You decided not to override Equals, which is fine, but you did override GetHashCode. GetHashCode is rarely used in user code, but mainly in containers which use hashing to store/retrieve elements. Overriding this method might break your code in these cases, since your Dimension is not immutable, and changing one's value while it is in a container, will make it 'disappear' from any such container he might be in.
Either make your class immutable, or delete this method. And if you decide to keep it, consider overriding Equals as well, they look so much better together...

String it!
You opted to not implement ToString in your code. This is problematic, since in many places in your user code, he may want to use traces or debug logs, and your decision to throw an error there will unnecessarily break their code. You might expect them to call dimension.[unit].ToString(), but what if they log a List of Dimension?

---

I notice that you chose not to put the DimensionConverter code in this post, which is a shame, because I can guess all the juicy parts are there, and you let us glimpse only on code with no complexity since it only delegates to that mysterious class :-(...

---

Update
Before I'll tackle the DimensionConverter, there are a couple of things I forgot to mention, which are very important, especially as you aim to have a "perfect" code:

Naming Conventions
C# naming conventions state that const members as well as all methods should be PascalCase. This is especially important in the case of you elusive const DimensionTypes internalUnitType. Consider the following two lines:

internalDecimalUnit = DimensionConverter.ConvertDimension(passedDimensionType, 
 passedInput, internalUnitType);
internalDecimalUnit = DimensionConverter.ConvertDimension(passedDimensionType, 
 passedInput, InternalUnitType);

Even SO's syntax highlighter sees the difference, which makes the intention of your constant a whole lot more apparent.

Use Decimal When used for human units (base 10), it is always advisable to use decimal instead of double. Doubles are notorious in their quirks when they are involved in arithmetic.

Unit tests
No self-respecting perfect code lacks a good fool-proof suite of unit tests! They make your code more stable, more readable and more maintainable. Educate your interns today on the importance of unit tests!

---

String it to me
You asked me what should ToString() return by default?
I think that on the most basic level, choose the format you feel is most appropriate in millimeters, or as architectural - the one most convenient to your users (remember to indicate which unit you are using in the resulting string!).
If you feel like being a little more elaborate, you can provide a global flag, which the user can set, which tells the ToString() method which is the default unit to render.

• jboy12: Thanks so much for your help! I really like your bug on mouseover, that is really cool. I was going to post the dimension converter code but I didnt want to overwhelm with code. is it acceptable to post multiple classes? if so, can I edit it in now?
• svick: I would be stronger about requiring Equals(): if you override GetHashCode(), you have to override Equals() as well. Otherwise, hash containers won't work for this type (because they use both methods).
• jboy12: I edited in the DimensionConverter class.
• jboy12: What should my toString return? I dont want to feed him values that he isnt expecting...
• Uri Agassi: @svick - yeah, when I started writing this section that's what I thought, but actually, there is no real harm in implementing GetHashCode() on its own (as long as it's returning the same thing each time...) - its default implementation is the instance's ObjectID which is as arbitrary as they come. Of course, although it is not harmful, it is definitely not helpful without Equals...

Answer 2

One obvious starting point: replace your code like this:

 public static bool operator ==(Dimension d1, Dimension d2)
 {
 //compare the two dimensions for equality
 //return a bool based on their relative values
 if (d1.internalDecimalUnit == d2.internalDecimalUnit) 
 {
 return true;
 }
 else
 {
 return false;
 }

...with the much simpler, more readable:

 return (d1.internalDecimalUnit == d2.internalDecimalUnit);

• jboy12: should I be overriding the .equals function?
• Tom Andersen: ON the readability of return (d1.internalDecimalUnit == d2.internalDecimalUnit), its not bad, but its not handy for setting break points in a debugger. What you then get is people rewriting code back to if statements while they are debugging, which can result in errors.

Answer 3

I'm tempted to call your == operator a perfect example of how not to do it: The concept of equality of floating-point numbers in the first place is somewhat of a poor concept in that e.g. IEEE definitions don't create behavior matching naive expectations---who would have known that x*(1.0/x) and 1.0 are often not equal (for very many values of x)?

Hence having such an operator without a documentation layer above the code that clearly defines what it is supposed to do or not to do is almost guaranteed to eventually create buggy code due to unfulfilled expectations, considering that even just converting units and immediately converting the result back will in many cases not result in a value equal to the starting value. The main problem here is that in the context of engineering measurements, equality could even more likely be seen as "essentially equal within engineering tolerances" than it already could be in the general use of floating numbers. I wouldn't even be surprised if in some context some intern naively expects it to mean "close enough to substitute a non-precision 4.8mm metric part with a 3/16in (nominally 4.76mm) imperial part." And maybe adjusts the code to improve it with regard to that interpretation, messing with some ball bearing engineer's assertion to check that his 2.49mm axle is really different from the 2.51mm bore hole for it. There you have your maintainability problem, complete with interns.

So what can one do to avoid this situation? I think the best one can hope for is to make users of the library aware of the problem, ideally in whatever becomes the preferred documentation, and (as last resort against people changing the code rather than their expectations) as comments in the code itself. As one (out of many) possibly approaches, I suggest the following "code" changes of the comments inside your == operator:

//compare the two dimensions for **floating-point** equality
//return a bool based on their relative values
//
//this is not helpful to determine if dimensions match within rounding accuracy
//(or within any other tolerance), and hence is rarely useful;
//
//see Microsoft (http://msdn.microsoft.com/en-us/library/dd183752.aspx):
//"Equality comparisons of floating point values (double and float) are problematic
//because of the imprecision of floating point arithmetic on binary computers."
//
//a partial solution, decimal floating-point arithmetic,
//is **NOT** used in this library.
//
//for most applications, consider explicitly using these alternatives:
// equality within an absolute tolerance tol: (abs(d1-d2) < tol)
// equality within a relative tolerance tol: (abs(d1-d2) < 0.5*tol*(abs(d1)+abs(d2))
// exact binary equality: (d1.GetHashCode() == d2.GetHashCode())

Of course a better solution would be to stop any problematic use before it gets to the point that someone digs into the source code to get to the bottom of it. I'm not sure how to do it (other than to improve the suggested comments to show up in documentation as well). You could indeed consider not having this operator in the first place, as svick suggests in his answer. That would be better than to sneakily, at run time, decide that this operator is undefined and never to return normally!

But whether having or not having the equality operator is better may depend on who uses it. I view having it much like I view an airplane having a particularly dangerous stall behavior: Such a plane would be very safe in the hands of a well-trained and alert pilot (who knows how to deal with it and that it is more likely than not that he will never want to make use of this "feature"), and arguably better than the alternative of one with such benign edge behavior that the more human among its pilots just cannot be convinced to pay much attention to its manual at all and may well end up less safe than they would otherwise be. So the best recommendation about keeping or not keeping the == operator probably depends on whether you want to write the perfect library for a circle of users having to go through extensive training, or for just anyone, including those interns maybe not previously familiar with either floating-point arithmetic in general or C# and your library in particular.

• jboy12: Great and helpful insight. What is the best of my options? delete it or replace the internal unit with the smallest thing possible and make the == true to so many significant digits? or something else?
• user40140: Since == is a good thing to provide (despite the dangers), I can't come up with a better answer than to clearly define what you want it to do (i.e. IEEE or something more special). I suppose I would be tempted to define it as IEEE (or C#) equality, and either provide some other method along the lines of bool equalsWithinTolerances(Dimension x, Dimension tol) or else point out in the doc that equivalent code is the more common use case.

Answer 4

#region internal variables

This is confusing, since the variables (or, more precisely, fields), are not internal, they are private.

---

DimensionTypes

That's a bad name. A value of that type represents a single "dimension type", so the type should be called accordingly: DimensionType. Also, wouldn't something like DimensionUnit be even better?

---

//internal Dimension type is set to milimeter to cause the least amount of rounding error when performing calculations.
const DimensionTypes internalUnitType = DimensionTypes.Millimeter;
//dimension value
private double internalDecimalUnit = 0.0;

You should decide whether to always type the private modified or never and stick with that, not write it for some members and omit it for others.

Also, internalDecimalUnit is quite confusing name, which you seem to be acknowledging by adding a comment that describes it in another way.

I don't see any advantage in having all private fields prefixed with internal (and again, it's also confusing, since they aren't really internal). If you want some prefix for fields, _ and m_ are common choices.

And the decimalUnit part is confusing too: it doesn't describe any unit, it describes a value. And I don't specifying that it's "decimal" makes much sense: either say explicitly which unit it is, or don't say it at all (see also below about changing the unit).

And it seems like the code is prepared if you want to change the unit type. But you also mark it as const, indicating that it will never change.

One more thing: the comment above internalUnitType seems to imply that you get the least amount of rounding errors with smaller units and thus larger numbers. But that's not how double works: unless you get close to its limits (approximately 10^-300 and 10³⁰⁰), the precision is always the same (around 15 decimal digits).

If you are really serious about rounding errors (you probably don't need to be, 15 digits is quite a lot), you would try to avoid conversions if possible. The most precise way to add two numbers in feet and print out the result in feet again is to perform all computations in feet, not convert to millimeters, add them, and then convert them back.

---

/// <summary>
/// Accepts any string value for input.
/// </summary>
public Dimension(string passedArchitecturalString)
{
 storeArchitecturalStringAsInternalUnit(passedArchitecturalString);
}

You don't have to specify passed on all parameters.

Also, does this constructor really accept any string value (e.g. "nějaká hodnota")? Or does it accept only values in some specific format? (Is that what "architectural string" is supposed to mean?)

And I think that the field should be actually assigned in the constructor, not in the helper method (in that case, the helper method would return the value to assign). But I can see the value in this approach too (less repetition).

---

public double Sixteenths
{
 get { return retrieveAsExternalUnit(DimensionTypes.Sixteenth); }
 set { storeAsInternalUnit(DimensionTypes.Sixteenth, value); }
}

I think this type would be better as immutable, i.e. it shouldn't have any setters. If you compare the following snippets, then I think the second one is better, even if it's longer:

desk.Length.Inches = 20;
desk.Length = Dimension.Inches(20);

Also, mutability causes big problems with dictionaries: if you use a Dimension as a key in a dictionary and then change its value, the dictionary won't work correctly anymore.

---

public static bool operator ==(Dimension d1, Dimension d2)
public static bool operator !=(Dimension d1, Dimension d2)
public static bool operator >(Dimension d1, Dimension d2)
public static bool operator <(Dimension d1, Dimension d2)
public override int GetHashCode()

If you overload ==, you really should also override Equals() and ideally also implement IEquatable<Dimension>. I'm actually surprised that overriding GetHashCode() without overriding Equals() compiles.

But keep pyramids' answer in mind and consider removing == completely.

Similarly, if you overload >, consider also implementing IComparable<Dimension>.

---

//compare the two dimensions together
//return a bool based on their relative values

Comments like this are pretty useless, there is certainly no need to repeat it over and over.

• jboy12: Fantastic comments, thanks so much. With your shortened public static bool operator ==(Dimension d1, Dimension d2) how does the compiler know to compare the private double internalDecimalUnit?
• svick: @jth41 I just included the method signatures in my answer, because I didn't need their bodies for that point. But you of course still have to write them.
• jboy12: Ok gotcha. I was very confused for a moment. VS does a lot for you, but that would be rediculous

Answer 5

I'm not going to discuss your code, but the original problem. If you want a general purpose, highly flexible dimensional analysis class, you need (for physical problems) three dimensions:

Length, Mass, Time (the Newtonian constants).

Using these you can define any physical constant.

A unit adds a coefficient to these three.

Let's take MKS as the default system.

public class Unit {
 private double c; // coeficient
 private int LENGTH, MASS, TIME;
 public Unit(String name) { ... }
}

Ok, so the unit "meter" corresponds to c=1, LENGTH = 1, MASS = 0, TIME = 0

the unit "foot" corresponds to c=1/3.048, LENGTH = 1, ...

The unit "kg" corresponds to C = 1, LENGTH = 0, MASS = 1, TIME = 0

With this class you can represent any physical quantity Area of an apartment in square meters?: C = 1, LENGTH = 2, MASS = 0, TIME = 0

For business applications, I find that a fourth dimension is useful, money. Unlike the others, money is not invariant. So if you specify in dollars, you need to specify what time the dollars are from. Still, ignoring inflation, you can use this to specify problems like:

house materials cost = 1 ton steel * 170ドル/ton + 352 kg fiberglass * 2ドル/kg + 270m^2 of tile * 32ドル/m^2 + ... Then quote the cost to the buyer in Euros based on the conversion ratio at the time.

To do this, just add a fourth dimension, money.

[1.0 3 1 1 -1] means: 1.0 m^3 kg s / $

Let's start by defining what you want to achieve, something you never really stated. I personally would like something like the following. I'm not really current in C# so I'm going to write it in C++ notation, which I'm pretty sure you can do in C#.

TypedQty houseArea(Dimension.MSQUARED, 500); // The area of the house in square meters
TypedQty houseHeight(Dimension.METERS, 10);
TypedQty houseVolume(Dimension.MCUBED, 2500);
TypedQty SteelMass(Dimension.KG, 1000);
TypedQty housePrice(Dimension.DOLLAR, 300e3);
TypedQty pricePerArea = houseArea/housePrice;
Dimension yenPerSquareInch = Dimension.YEN / Dimension.DOLLAR * Dimension.INCH * Dimension.INCH / Dimension.MSQUARED;
pricePerArea.convert(yenPerSqInch).print();

Answer 6

Since my previous answer is already long, I've decided to write a separate answer for the second class, to give it the proper respect...

I will not repeat the virtue in succinct names (I believe that ToMillimeters(string architectural) conveys as much information as ConvertArchitectualStringtoMillimeters(String arch)).

Make your regular expression readable
The single most important part of the first method is the regular expression. It is also the most hard to read...
Regular expression are hard to read, but there are ways to make them more readable. You can use comments to your expression, writing it in a multiline fashion, to make it a lot more readable:

String expr = @"^\s*
 (?<minus>-)? # optional minus sign
 \s*(((?<feet>\d+)(?<inch>\d{2})(?<sixt>\d{2}))| # long number format (121103 == 12 Feet and 11 Inches and 3 sixteenths)
 (
 (?<feet>[\d.]+)')?[\s-]* # 12'
 ((?<inch>\d+)?[\s-]* # 11''
 ((?<numer>\d+)/(?<denom>\d+))?\")? # ...or 11 3/16''
 ) # 12' 11 3/16''
 \s*$";
 Match m = new Regex(expr, RegexOptions.IgnorePatternWhitespace).Match(arch);

Meaningful names
I was surprised to see a variable name called num in your, otherwise verbose, code. You add insult to injury, when the first thing you do in that method is change its value and meaning!
I had to read the code a few times to understand how you can convert from millimeters to feet by dividing by 12...
Call the parameter millimeters, and create a new variable - inches inside the method.

Declare variables only when they are needed
Don't declare a bucket load of variables at the beginning of a long method. This will cause the reader to continuously scroll up and down to see what the variable might mean.
Also, don't give an unused default value when declaring variables. You can hide bugs when trying to use the variables before they are set. And again, bug of the day, can you spot the bug caused by this in ConvertDecimalDimensionToArchitecturalString? Don't peek!

All converted values will be from 0.0

Declare variables as late as possible in the method, and don't give meaningless default values:

int feet = (int)inches / 12;
// ...
string feetString = (feet == 0) ? "" : Convert.ToString(feet);

(see what I did there with FeetString? read about naming conventions in my other answer...)

Where possible, skip the variable altogether, and inline it:

public static string ConvertDecimalDimensionToArchitecturalString(DimensionType typeConvertingFrom, double passedValue)
{
 return ConvertMillimetersToArchitecturalString(
 ConvertDimension(DimensionType.Millimeter, passedValue, typeConvertingFrom));
}

When comments are forgotten
In ConvertDimension you have the following comments:

//first convert value passedValue to inches
//Now convert the value from inches to the desired output

But your code converts to and from millimeters...

Method too long
It seems that ConvertDimension method is too long. Its usage in Dimension class is always from a known unit to millimeters, or vice versa. I believe that breaking it down to methods doing just that is preferable, and more readable:

public static double FromThirtySecond(double thirtySecond)
{
 return (thirtySecond / 32) * 0.0393701;
}
public static double FromSixteenth(double sixteenth) 
{
 return FromThirtySecond(sixteenth * 2);
}
// ... snip ...
public static double ToMile(double millimeters)
{
 return millimeters / 1609344;
}
public static double ToKilometer(double millimeters)
{
 return millimeters / 1000000;
}

Now the properties in Dimension class can also look a lot more readable:

public double Sixteenth
{
 get { return DimensionConverter.ToSixteenth(intrinsicValue); }
 set { intrinsicValue = DimensionConverter.FromSixteenth(value); }
}

• Dov: Why would you want to write hardcoded methods for every potential unit? There are a huge number even if you "only" consider the common ones: mile, km, meter, yard, foot, inch, cm, mm mile
• Uri Agassi: @Dov, since the original method had nothing generic or dynamic about it, it is basically a huge switch case, replacing the long unreadable method with a lot of small, readable, and more testable methods has no real downside... You don't 'save' any code writing by lumping it all up in a big method. See also real-life example in C# msdn.microsoft.com/en-us/library/system.convert(v=vs.110).aspx
• Dov: see my answer. I am arguing for a rather different total solution
• Uri Agassi: @Dov, as you testify yourself about your answer - it does not discuss the OP's code. Since this is codereview.stackexchange.com, I prefer to stay with the OP's objectives and strategy, and help him write better code.

Answer 7

I'm going to write a separate answer with code the way I think it should be, but since I'm not a C# programmer there will be small differences in syntax. The overall design should be the same.

First, conventions: a Dimension in physics is the kind of unit. A unit has a particular scale in that dimension. For example, a Dimension of length is a generic thing, but there are meters, feet, etc. I realize your field is architecture, but even so you deal with at least L, L^2, L^3 if not more, so I will stick to this convention.

import java.util.*;
public class Unit {
 private String name;
 private String abbrev;
 private int LENGTH, MASS, TIME, MONEY;
 private double c;
 private static HashMap<String, Unit> unitsByName;
 private static HashMap<Integer, Unit> unitsByDimension;
 // these static units are declared at compile time for error checking
 // if all units are put here, that makes the code fairly big
 //MKS (Systeme Internationale the engineering superset of the metric system)
 public static Unit METER, KG, SEC;
 // derivative common units
 public static Unit NM, uM, MM, CM, KM, GRAM, TONNE, MINUTE, HOUR, DAY, YEAR;
 //English
 public static Unit INCH, FOOT, YARD, MILE, POUNDMASS;
 //Money
 public static Unit USD, JPY, EUR;
 static {
 unitsByName = new HashMap<String, Unit>(128);
 unitsByDimension = new HashMap<Integer, Unit>(128);
 METER = new Unit("meter", "m", 1,0,0,0);
 KG = new Unit("kilogram", "kg", 0,1,0,0);
 SEC = new Unit("second", "s", 0,0,1,0);
 USD = new Unit("dollar", "$", 0,0,0,1);
 NM = new Unit("nanometer", "nm", 1e-9,1,0,0,0);
 uM = new Unit("micrometer", "um", 1e-6,1,0,0,0);
 MM = new Unit("millimeter", "mm", 1e-3,1,0,0,0);
 CM = new Unit("centimeter", "cm", 0.01,1,0,0,0);
 KM = new Unit("kilometer", "km", 1000.0,1,0,0,0);
 GRAM = new Unit("gram", "g", 0.001,0,1,0,0); // scaled in terms of MKS kilogram
 TONNE = new Unit("tonne", "T", 1000,0,1,0,0); // scaled in terms of MKS kilogram
 INCH = new Unit("inch", "in", 0.0254,1,0,0,0);
 FOOT = new Unit("foot", "ft", 12*0.0254,1,0,0,0); // don't combine constants, keep it understandable
 YARD = new Unit("yard", "yd", 36*0.0254,1,0,0,0);
 }
 public int hashCode() {
 return ((LENGTH * 4 + MASS) * 4 + TIME) * 4 + MONEY;
 }
 public Unit(String name, String abbrev, double c, int len, int mass, int time, int money) {
 this.name = name;
 this.abbrev = abbrev;
 this.c = c;
 LENGTH=len; MASS = mass; TIME = time; MONEY = money;
 unitsByName.put(name, this);
 unitsByDimension.put(hashCode(), this);
 }
 public Unit(String name, String abbrev, int len, int mass, int time, int money) {
 this(name, abbrev, 1.0, len, mass, time, money);
 }
 public Unit(double c, int len, int mass, int time, int money) {
 this("", "", c, len, mass, time, money);
 }
 public boolean equals(Object other) {
 Unit o = (Unit)other;
 // Hmmm.. thinking of what equality of dimension really means...
 return LENGTH == o.LENGTH && MASS == o.MASS && TIME == o.TIME && MONEY == o.MONEY &&
 Math.abs(c - o.c) / c < .001;
 // for units at least, an accuracy of 0.1% is pretty much a guarantee. Haven't thought about this much though
 }
 public Unit mult(Unit u) {
 u = new Unit(c*u.c, LENGTH+u.LENGTH, MASS+u.MASS, TIME+u.TIME, MONEY+u.MONEY);
 Unit stdUnit = unitsByDimension.get(u);
 return (stdUnit != null) ? stdUnit : u;
 }
}

The above code is not quite done, but it shows the idea and the elegance of this approach. A DimensionalQty is a pair (number, Unit). I have not shown that.