patterncsharpMinor
Is this a good implementation of a simple "Size" value type
Viewed 0 times
thissimplesizevaluetypegoodimplementation
Problem
I have created the value type below to represent the desired size for an image. The
Is this a good implementation of a value type, especially regarding the
What are your thoughts on the use of the private constructor to create the
```
using System;
public struct Size : IEquatable
{
private readonly int _height;
private readonly int _width;
private readonly bool _default;
public static readonly Size Default = new Size(true);
public int Height
{
get { return _height; }
}
public int Width
{
get { return _width; }
}
public bool IsDefault
{
get { return _default; }
}
private Size(bool isDefault)
{
_height = 0;
_width = 0;
_default = isDefault;
}
public Size(int width, int height)
{
_height = height;
_width = width;
_default = false;
}
public override bool Equals(object obj)
{
return this.Equals((Size)obj);
}
public bool Equals(Size other)
{
if (this._default && other._default)
return true;
if (this._default && !other._default)
return false;
if (!this._default && other._default)
return false;
return this.Width == other.Width && this.Height == other.Height;
}
public static bool operator ==(Size left, Size right)
{
return left.Equals(right);
}
public static bool operator !=(Size left, Size right)
{
return !left.Equals(right);
}
public static Size Parse(string input)
{
var parts = input.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
if (parts.Length != 2)
throw new FormatException("Input string was not in a correct format.");
var x = int
Size.Default is used in situations where the image is required in the size it was supplied in, i.e. the image shouldn't be resized.Is this a good implementation of a value type, especially regarding the
IEquatable implementation?What are your thoughts on the use of the private constructor to create the
Default instance?```
using System;
public struct Size : IEquatable
{
private readonly int _height;
private readonly int _width;
private readonly bool _default;
public static readonly Size Default = new Size(true);
public int Height
{
get { return _height; }
}
public int Width
{
get { return _width; }
}
public bool IsDefault
{
get { return _default; }
}
private Size(bool isDefault)
{
_height = 0;
_width = 0;
_default = isDefault;
}
public Size(int width, int height)
{
_height = height;
_width = width;
_default = false;
}
public override bool Equals(object obj)
{
return this.Equals((Size)obj);
}
public bool Equals(Size other)
{
if (this._default && other._default)
return true;
if (this._default && !other._default)
return false;
if (!this._default && other._default)
return false;
return this.Width == other.Width && this.Height == other.Height;
}
public static bool operator ==(Size left, Size right)
{
return left.Equals(right);
}
public static bool operator !=(Size left, Size right)
{
return !left.Equals(right);
}
public static Size Parse(string input)
{
var parts = input.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
if (parts.Length != 2)
throw new FormatException("Input string was not in a correct format.");
var x = int
Solution
I'm not sure what the point is for making a distinction between a size and the default size. But you shouldn't expose the default value as a field, use a property. That will give you much more flexibility should you decide to do other things when getting the value. No need for a backing field, it's a value type and it isn't all that complex.
Consider renaming
Your
Your
Any time you override the equals method, you must override the
While we're on the subject of overriding methods, you should consider overriding the
Your
public static Size Default
{
get { return new Size(true); }
}Consider renaming
_default to something that would indicate it is a bool. I'd usually go with something like _isDefault. You already did that for the constructor parameter and property, I don't know why you didn't do the same for the field.Your
Equals() method should not ever throw exceptions. What would happen if you passed in something that was not a Size?public override bool Equals(object other)
{
if (other is Size)
return Equals((Size)other);
return false;
}Your
Equals(Size) method is a little unusual. You can merge the second and third conditions into a single this._isDefault != other._isDefault. It might make more sense to combine that test with the tests for width and heights. And personally, I wouldn't mix accessing fields and properties within a single method, I'd choose one or the other unless the properties had other logic that I needed to be fired.public bool Equals(Size other)
{
if (this._isDefault && other._isDefault)
return true;
return this._isDefault == other._isDefault &&
this._width == other._width &&
this._height == other._height;
}Any time you override the equals method, you must override the
GetHashCode() method too. Use an implementation something like this:public override int GetHashCode()
{
unchecked
{
var hashCode = 79;
hashCode = hashCode * 97 + _isDefault.GetHashCode();
hashCode = hashCode * 97 + _width.GetHashCode();
hashCode = hashCode * 97 + _height.GetHashCode();
return hashCode;
}
}While we're on the subject of overriding methods, you should consider overriding the
ToString() method as well to return something reasonable.Your
Parse() method should do more validation than that IMHO. You make the assumption that the input will be a comma separated pair of values. You check if you have a pair which is good, but you should explicit perform checks to make sure the values are integer numbers (i.e., digits). It's not as useful receiving a generic format exception coming from the int.Parse() method than if it came from your Parse() method. On a minor note, use width and height for the local variables and not x and y... that's not what they're representing.Code Snippets
public static Size Default
{
get { return new Size(true); }
}public override bool Equals(object other)
{
if (other is Size)
return Equals((Size)other);
return false;
}public bool Equals(Size other)
{
if (this._isDefault && other._isDefault)
return true;
return this._isDefault == other._isDefault &&
this._width == other._width &&
this._height == other._height;
}public override int GetHashCode()
{
unchecked
{
var hashCode = 79;
hashCode = hashCode * 97 + _isDefault.GetHashCode();
hashCode = hashCode * 97 + _width.GetHashCode();
hashCode = hashCode * 97 + _height.GetHashCode();
return hashCode;
}
}Context
StackExchange Code Review Q#11617, answer score: 5
Revisions (0)
No revisions yet.