views:

294

answers:

6

I showed this struct to a fellow programmer and they felt that it should be a mutable class. They felt it is inconvenient not to have null references and the ability to alter the object as required. I would really like to know if there are any other reasons to make this a mutable class.

[Serializable]
public struct PhoneNumber : IEquatable<PhoneNumber>
{
    private const int AreaCodeShift = 54;
    private const int CentralOfficeCodeShift = 44;
    private const int SubscriberNumberShift = 30;
    private const int CentralOfficeCodeMask = 0x000003FF;
    private const int SubscriberNumberMask = 0x00003FFF;
    private const int ExtensionMask = 0x3FFFFFFF;


    private readonly ulong value;


    public int AreaCode
    {
        get { return UnmaskAreaCode(value); }
    }

    public int CentralOfficeCode
    {
        get { return UnmaskCentralOfficeCode(value); }
    }

    public int SubscriberNumber
    {
        get { return UnmaskSubscriberNumber(value); }
    }

    public int Extension
    {
        get { return UnmaskExtension(value); }
    }


    public PhoneNumber(ulong value)
        : this(UnmaskAreaCode(value), UnmaskCentralOfficeCode(value), UnmaskSubscriberNumber(value), UnmaskExtension(value), true)
    {

    }

    public PhoneNumber(int areaCode, int centralOfficeCode, int subscriberNumber)
        : this(areaCode, centralOfficeCode, subscriberNumber, 0, true)
    {

    }

    public PhoneNumber(int areaCode, int centralOfficeCode, int subscriberNumber, int extension)
        : this(areaCode, centralOfficeCode, subscriberNumber, extension, true)
    {

    }

    private PhoneNumber(int areaCode, int centralOfficeCode, int subscriberNumber, int extension, bool throwException)
    {
        value = 0;

        if (areaCode < 200 || areaCode > 989)
        {
            if (!throwException) return;
            throw new ArgumentOutOfRangeException("areaCode", areaCode, @"The area code portion must fall between 200 and 989.");
        }
        else if (centralOfficeCode < 200 || centralOfficeCode > 999)
        {
            if (!throwException) return;
            throw new ArgumentOutOfRangeException("centralOfficeCode", centralOfficeCode, @"The central office code portion must fall between 200 and 999.");
        }
        else if (subscriberNumber < 0 || subscriberNumber > 9999)
        {
            if (!throwException) return;
            throw new ArgumentOutOfRangeException("subscriberNumber", subscriberNumber, @"The subscriber number portion must fall between 0 and 9999.");
        }
        else if (extension < 0 || extension > 1073741824)
        {
            if (!throwException) return;
            throw new ArgumentOutOfRangeException("extension", extension, @"The extension portion must fall between 0 and 1073741824.");
        }
        else if (areaCode.ToString()[1] == '9')
        {
            if (!throwException) return;
            throw new ArgumentOutOfRangeException("areaCode", areaCode, @"The second digit of the area code cannot be greater than 8.");
        }
        else
        {
            value |= ((ulong)(uint)areaCode << AreaCodeShift);
            value |= ((ulong)(uint)centralOfficeCode << CentralOfficeCodeShift);
            value |= ((ulong)(uint)subscriberNumber << SubscriberNumberShift);
            value |= ((ulong)(uint)extension);
        }
    }


    public override bool Equals(object obj)
    {
        return obj != null && obj.GetType() == typeof(PhoneNumber) && Equals((PhoneNumber)obj);
    }

    public bool Equals(PhoneNumber other)
    {
        return this.value == other.value;
    }

    public override int GetHashCode()
    {
        return value.GetHashCode();
    }

    public override string ToString()
    {
        return ToString(PhoneNumberFormat.Separated);
    }

    public string ToString(PhoneNumberFormat format)
    {
        switch (format)
        {
            case PhoneNumberFormat.Plain:
                return string.Format(@"{0:D3}{1:D3}{2:D4}{3:#}", AreaCode, CentralOfficeCode, SubscriberNumber, Extension).Trim();
            case PhoneNumberFormat.Separated:
                return string.Format(@"{0:D3}-{1:D3}-{2:D4} {3:#}", AreaCode, CentralOfficeCode, SubscriberNumber, Extension).Trim();
            default:
                throw new ArgumentOutOfRangeException("format");
        }
    }

    public ulong ToUInt64()
    {
        return value;
    }


    public static PhoneNumber Parse(string value)
    {
        var result = default(PhoneNumber);
        if (!TryParse(value, out result))
        {
            throw new FormatException(string.Format(@"The string ""{0}"" could not be parsed as a phone number.", value));
        }
        return result;
    }

    public static bool TryParse(string value, out PhoneNumber result)
    {
        result = default(PhoneNumber);

        if (string.IsNullOrEmpty(value))
        {
            return false;
        }

        var index = 0;
        var numericPieces = new char[value.Length];

        foreach (var c in value)
        {
            if (char.IsNumber(c))
            {
                numericPieces[index++] = c;
            }
        }

        if (index < 9)
        {
            return false;
        }

        var numericString = new string(numericPieces);
        var areaCode = int.Parse(numericString.Substring(0, 3));
        var centralOfficeCode = int.Parse(numericString.Substring(3, 3));
        var subscriberNumber = int.Parse(numericString.Substring(6, 4));
        var extension = 0;

        if (numericString.Length > 10)
        {
            extension = int.Parse(numericString.Substring(10));
        }

        result = new PhoneNumber(
            areaCode,
            centralOfficeCode,
            subscriberNumber,
            extension,
            false
        );

        return result.value != 0;
    }

    public static bool operator ==(PhoneNumber left, PhoneNumber right)
    {
        return left.Equals(right);
    }

    public static bool operator !=(PhoneNumber left, PhoneNumber right)
    {
        return !left.Equals(right);
    }

    private static int UnmaskAreaCode(ulong value)
    {
        return (int)(value >> AreaCodeShift);
    }

    private static int UnmaskCentralOfficeCode(ulong value)
    {
        return (int)((value >> CentralOfficeCodeShift) & CentralOfficeCodeMask);
    }

    private static int UnmaskSubscriberNumber(ulong value)
    {
        return (int)((value >> SubscriberNumberShift) & SubscriberNumberMask);
    }

    private static int UnmaskExtension(ulong value)
    {
        return (int)(value & ExtensionMask);
    }
}

public enum PhoneNumberFormat
{
    Plain,
    Separated
}
+7  A: 

I think it's fine to keep it as an immutable struct - but I would personally just use separate variables for each of the logical fields unless you're going to have huge numbers of these in memory at a time. If you stick to the most appropriate type (e.g. ushort for 3-4 digits) then it shouldn't be that expensive - and the code will be a heck of a lot clearer.

Jon Skeet
I based the design off of `DateTime` whether that's a good thing or bad.
ChaosPandion
You really think the code is that bad? I felt the `struct` as a whole was pretty simple.
ChaosPandion
Combining 5 logical values into one is never exactly a "simple" approach.
Jon Skeet
Technically, it is one logical value with sub-values but your point still stands.
ChaosPandion
That's why it's one *struct*. How the struct stores its data is an implementation detail. In *general*, you'll find you have less problems if you store individual values separately. Trying to combine them almost always seems to lead to problems.
kyoryu
@kyoryu - So the consensus is that storing the components separately is the better way to go?
ChaosPandion
@ChaosPandion: If I got a code review with that code, I would say to store the data in separate fields. There's really no benefit not to. I'd guess that DateTime does so because of underlying implementation details.
kyoryu
+2  A: 

I agree that this should be an immutable type. But why this struct should implement a ICLoneable and IEquatable interface? It is a value type.

Al Bundy
Hmm, you are right about the `IClonable` but even DateTime implements `IEquatable`.
ChaosPandion
It's absolutely correct for it to implement `IEquatable<PhoneNumber>`. Why wouldn't it? Why would you *not* want to be able to check whether one phone number is equal to another, and hash them in a set etc?
Jon Skeet
yes, IEquatable is fine :)
Al Bundy
+2  A: 

Personally, I feel that leaving this as an immutable struct is a very good thing. I would not recommend changing it to a mutable class.

Most of the time, in my experience, people wanting to avoid immutable structs are doing this from laziness. Immutable structs force you to recreate the struct will full parameters, but good constructor overloads can help tremendously here. (For an example, look at this Font constructor - even though it's a class, it implements a "clone everything but this variable" pattern that you can duplicate for your common fields that need changing.)

Creating mutable classes introduces other issues and overhead that I would avoid unless necessary.

Reed Copsey
+15  A: 

A program that manipulates a phone number is a model of a process.

Therefore, make things which are immutable in the process immutable in the code. Make things which are mutable in the process mutable in the code.

For example, a process probably includes a person. A person has a name. A person can change their name while retaining their identity. Therefore, the name of a person object should be mutable.

A person has a phone number. A person can change their phone number while retaining their identity. Therefore, the phone number of a person should be mutable.

A phone number has an area code. A phone number CANNOT change its area code and retain its identity; you change the area code, you now have a different phone number. Therefore the area code of a phone number should be immutable.

Eric Lippert
So are you saying a mutable field on `Person` with an immutable phone number is the wrong direction?
ChaosPandion
Let me try again. A string is not mutable. To turn "Anderson" into "Andreeson" you don't modify "Anderson"; you throw it away entirely and replace the whole thing. The *name field* is mutable; the *string that contains any given name* is not. Same with a phone number. A phone number is not mutable; to turn 519-555-1212 into 206-555-1212 you don't mutate the first one, you throw it away and start over. But the *phone number field* on a person is mutable.
Eric Lippert
Got it, we are on the same page now.
ChaosPandion
Since, you're here maybe you could give your opinion on the design of the `struct`?
ChaosPandion
+1  A: 

Perhaps your co-worker could be satisfied by a set of methods to allow individual fields to be easily "changed" (resulting in a new instance with the the same values as the first instance except for the new field).

public PhoneNumber ApplyAreaCode(int areaCode)
{
  return new PhoneNumber(
    areaCode, 
    centralOfficeCode, 
    subscriberNumber, 
    extension);
}

Also, you could have a special case for an "undefined" phone number:

public static PhoneNumber Empty
{ get {return default(PhoneNumber); } }

public bool IsEmpty
{ get { return this.Equals(Empty); } }

The "Empty" property gives a more natural syntax than either "default(PhoneNumber) or new PhoneNumber()" and allows for the equivalent of a null check with either "foo == PhoneNumber.Empty" or foo.IsEmpty.

Also... In your TryParse don't you mean to

return result.value != 0;
StarLite
ooops, nice catch.
ChaosPandion
I like the idea of the `Empty` property. The helper methods however I am not so sure.
ChaosPandion
A: 

Nullability can be easily handled via PhoneNumber?

micahtan
Shouldn't this be a comment or am I missing something?
ChaosPandion
"They felt it is inconvenient not to have null references and the ability to alter the object as required."
micahtan