current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I have a class that I use to represent a distance called Dimension.

Here is its four constructors:

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

    /// <summary>
    /// Accepts standard types for input.
    /// </summary>
    public Dimension(DimensionType passedDimensionType, double passedInput)
    {
        storeAsInternalUnit(passedDimensionType, passedInput);
    }

    /// <summary>
    /// copy constructor - create a new Dimension with the same _intrinsicValue as the passed Dimension
    /// </summary>
    public Dimension(Dimension passedDimension)
    {
        _intrinsicValue = passedDimension._intrinsicValue;
    }

    /// <summary>
    /// Zero Constructor
    /// </summary>
    public Dimension()
    {
        _intrinsicValue = 0;
    }

This question gives some more background on my Dimension Class.

Because it is a class that I use so often and it usually uses inches. I had to write out the one of the full constructors every time that I wanted to use it.

Dimension inchDimension = new Dimension(DimensionType.Inch, 14.1875);

This became a major pain when creating things like my 3 dimensional Point object:

Dimension xDimension = new Dimension(DimensionType.Millimeter, 0);
Dimension yDimension = new Dimension(DimensionType.Millimeter, 0);
Dimension zDimension = new Dimension(DimensionType.Millimeter, 0);

        Point p = new Point(xDimension, yDimension, zDimension);

So I created a class to generate these objects called DimensionGenerator.

public class DimensionGenerator
{

    /// <summary>
    /// Default generator sets to inches
    /// </summary>
    public DimensionGenerator()
    {
        _internalType = DimensionType.Inch;
    }

    /// <summary>
    /// This constructor allows you to specify a static type and change it later
    /// </summary>
    /// <param name="wantedType">DimensionType to use for now</param>
    public DimensionGenerator(DimensionType wantedType)
    {
        _internalType = wantedType;
    }

    DimensionType _internalType;
    public DimensionType DimensionOutputType
    {
        get { return _internalType; }
        set { _internalType = value; }
    }

    public Dimension MakeDimension(double passedValue)
    {
        return new Dimension(_internalType, passedValue);
    }
}

I seem to use it a LOT. its less typing to just create on of these instead of using a Dimension constructor. Especially when writing Unit Tests where numbers may not really matter and all I care about are their equality or etc. Isn't it terribly wasteful to create a new one of these in most methods in my application? Should I make a bunch of static DimensionGenerators that only output a specific type? What should I consider when having a class like this?

share|improve this question
    
Your question is pretty much ok but next time please try to include more code about what you are really trying to achieve. In this question you wanted to compare two Dimensions and you don't have a single method about converting dimensions nor comparing them. –  Bruno Costa Jul 8 '14 at 22:35

1 Answer 1

up vote 2 down vote

The concept/pattern that you are trying to apply to DimensionGenerator is actually a factory. You can approach this problem in two major ways, the factory which is what you were trying to do and by subclassing.

To make the factory pattern correct your dimension class must not have any public constructor and all instances shall be created by calling the factory method:

public class DimensionFactory
{
    public Dimension MakeInchDimension(double distance){
        return new Dimension(DimensionType.Millimeter, distance);
    }
    //more methods for other types
}

Should there be many of DimensionType that would be impractical to define a method for each type then you could do the factory like you already pretty much do.

public class DimensionFactory{
    private readonly DimensionType dimensionType;
    public DimensionFactory(DimensionType dimensionType){
        this.dimensionType = distance;
    }
    public Dimension MakeDimension(double distance){
        return new Dimension(dimensionType, distance);
    }  
}

In this approach you will end up with a factory object per each dimension type and you will have a generic Make method for all types, this is a big pro since all types are built in the same way. In the first approach you would have to change the interface if you wanted to add a new dimension type in this approach you don't have to do anything at all.

You could also sub class Dimension so you would have one class per DimensionType, once again this approach is only feasible if there are few DimesnionTypes. in this approach Dimension shall be abstract:

public InchDimension : Dimension{
     public InchDimension(double distance):base(DimensionType.Inch, distance){}
}

Edit Asside from creating dimension your major concern was about comparing them. I'll assume that you can only compare objects of the same dimension and that you can convert any dimension to any other dimension. This way you are detaching conversion from comparison and comparison algorithm is solved easier.

public Dimension{ 
   public DimensionType DimensionType{get; private set;}
   public Dimension(DimensionType type, double value){
       DimensionType = type;
       Value = value;
   }
   public double Value{get; set;}
   public Dimension ConvertTo(DimensionType type){
      //if this dimension type is not consersible to type then throw NotSupportedException()
      //else do the convertion logic between this type and the other.
   }
}

public DimensionComparer : IEqualityComparer<Dimension>{
   public bool Equals(Dimension d1, Dimension d2){
       if(d1.DimensionType != d2.DimensionType){
           throw new NotSupportedException("cannot compare different dimmension types, please convert your dimension first");
       }
       return d1.Value == d2.Value;
   }
   public int GetHashCode(Dimension d){
      return d.Value ^ d.DimensionType.GetHashCode();
   }
}
share|improve this answer
    
An additional question about constructors in general. Is it a terribly bad idea to have what I call "Zero Constructors" like this Dimension d = new Dimension(); where it just creates a dimension that is zero? I was tempted to use those with the classes like dimension and point –  jth41 Jul 8 '14 at 21:41
    
I think it is a bad idea because your class definition doesn't make any sense at all without a double value. The same applies to the struct Point, it always have x and y. Plus you can't complain about just passing a 0 to your constructor. Althought Default constructors themselves aren't always bad, I think that in this scenario they don't fit. –  Bruno Costa Jul 8 '14 at 21:49
    
it just happens that the double value of 0 is the only time where meters, inches, feet and such are = –  jth41 Jul 8 '14 at 21:53
    
I see I should have read your question better and see your concern about equality I'll update my answer, just a minute. –  Bruno Costa Jul 8 '14 at 21:57
    
Btw the official name is constructor without parameters not "zero constructors" nor default constructor even if some people (like me) call them by that name often (wrongly). –  Bruno Costa Jul 8 '14 at 22:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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