Which design option is more suitable for auto-correction on construction?

Question

Trying to decipher an appropriate OO design to implement. The basic scenario is that you have a PstnNumber which is essentially a 10 digit phone number that always starts with 0 (e.g. 0195550000). A rule has been introduced to allow auto-correcting of a number if the leading 0 is missing (e.g. 195550000).

START EDIT

I realised the original question may have been misunderstood (thankyou kindly to those whom have answered already), so I have edited to try and better explain the scenario.

END EDIT

I started playing with some preliminary concepts and then thought I would ask if there was a more appropriate way to go or do one of these suffice (on some level)?

Concept 1

public class PstnNumber
{
    public virtual string Number { get; set; }

    public PstnNumber() { }

    public PstnNumber(string number)
    {
        this.Number = number;
    }
}

public class AutoFormattedPstnNumber : PstnNumber
{
    public override string Number
    {
        get { return base.Number; }
        set { base.Number = value.PadLeft(10, '0'); }
    }

    public AutoFormattedPstnNumber() : base() { }

    public AutoFormattedPstnNumber(string number)
    {
        this.Number = number;
    }
}

Concept 2 (removed)

Concept 3

public class PstnNumber
{
    public bool AutoCorrect { get; set; }

    private string number;
    public virtual string Number
    {
        get { return (this.AutoCorrect) ? this.number.PadLeft(10, '0') : this.number; }
        set { this.number = value; }
    }

    public PstnNumber() : this(false) { }

    public PstnNumber(bool autoCorrect)
    {
        this.AutoCorrect = autoCorrect;
    }

    public PstnNumber(string number) : this(false)
    {
        this.Number = number;
    }

    public PstnNumber(string number, bool autoCorrect) : this(autoCorrect)
    {
        this.Number = number;
    }
}

I think Concept 1 may violate the Liskov Substitution rule because the subclass changes the behaviour of the Number property (happy to learn if I've misunderstood that).

Any alternative suggestions would be received happily.

Answer 1

do you have to do the autoformatting when the object is instantiated? If not, what about:

  public class PstnNumber
  {
    public virtual string Number { get; set; }
    public PstnNumber() { }
    public PstnNumber(string number) { this.Number = number; }
    public AutoFormatNumber { get { return Numer.PadLeft(10, '0'); } }
  }

Answer 2

In Option 1 and Option 2, you aren't preserving the original number anyway, rendering the subclass worthless (except to know that it was autoformatted at some point, which doesn't seem like useful information). The alternative to make these Options more useful would be to format on Get instead of Set.

Option 3 is therefore the preferred pattern out of these three options, but I would also ask - why can't the PstnNumber also simply detect the number of digits, and autoformat accordingly?

Answer 3

If you follow the rules - there is one that says that "each routine (read class) should do only one thing and do it well".

According to that I would make PstnNumber just hold the number, and create some sort of factory that produces the right number.

Doing both in the same class means that you are weaving domain logic and representation. I prefer them separated.

Answer 4

I'd ask why your class name is so cryptic. "Number" is clear to me, and "P" suggests "phone", but what's the "stn" telling me? A few extra keystrokes would make this class more self-documenting.

I'd also ask about the logic of a default constructor that does not initialize the underlying data members to some value. I think a default constructor should have a sensible default value if possible.

I feel like option 1 is overkill. I don't think inheritance is making this model clearer or better. I don't see how it breaks Liskov substitution, which demands that you can use the subclass in any situation that calls for a base class. The methods map 1:1 as far as I can see. How is Liskov violated?

Option 2 says these are two separate classes with no relationship. That doesn't seem right to me.

All this work suggests that your problem will require that you use both classes. You'll have situations where the leading zero is NOT required and others where it is. Is that true? Or are you always going to require the leading zero?

I don't care for any of your options. I'd prefer an interface or a static factory or even modifying the class you have to anything you've suggested. It feels like a mere formatting issue. Do you store the number with the leading zero? If not, maybe it's just a view concern.

Answer 5

I am not familiar with c#, but I'd do this:

public class PstnNumber {
  readonly string number;

  public PstnNumber(string number) {
    this.number = number;
  }

  public string getNumber() {
    return number;
  }

  static public PstnNumber createNumber(string number) {
    return new PstnNumber(number.PadLeft(10, '0'));
  }
}

Of course if I knew how Properties work, I'd probably do it differently :)

Answer 6

avoid getter-setter-surprise
Avoid getters returning a different value than the one accepted by the setter. Imagine the following snippet:

if (input.Value != current.Number)
{
   NumberChangedAgain = true;
   current.Number = input.Value;
}

A simple solution would be to make PstnNumber immutable:

 temp = PstnNumber.FromString(input.Value);
 if (temp != current) { ... }

canonical format
If some data has different representations, there is a lot of advantage to storing it in a canonical representation, and move the format conversions to factory functions and getters / formatters. For example, you don't need to test comparison for short vs. long, long vs. short, short vs. short, long vs. long.

different aspects
Do you need the distinction between an "autoformatted" and a "normal" number, or is this merely a question of input and output - i.e.

• does display format (short or long) depend on how the number was entered, or on where it is displayed?
• is 0195550000 == 195550000 ?

I'd prefer to fold both classes into one if possible (i.e. when "entered with or without 0 can be forgotten"):

public class PstnNumber 
{  
   private string m_number; // always in long format
   public static PstnNumber(string s)  { ... } // accepts short and long form

   public string Number { get { return m_number; } }
   public string AutoFormatted { { get { ... } }
}

Otherwise I'd go with Option 3, but always store the long format in m_number.

Answer 7

Do you have a really strong reason to have a setter and not have your members final? If not, that's probably a bigger problem than any other variation between the three.

So I'd go for a stateless #3 which means making the number final and gettng rid of the autoFormat variable.

For simplicity I'd just have a getNumberRaw and getNumberFormatted

Better yet, you could have getNumberRaw and getNumber(formatType) where formatType actually contains the code that formats the number since the format may change again in the future and combining formatting (view) with your phone number (model) isn't optimal.

(PS/EDIT): just the fact that a phone number can change is NOT a good reason to have a setter! Creating a new phone number object and replacing the old one will almost always work!

Answer 8

I would go with a much simpler version, overriding the ToString method, or even, creating an ToString overload that receives the bool parameter indicating that the number should be formatted.