9
\$\begingroup\$

Let's assume I have a class PhoneNumber which accepts phone numbers in the format "123-456-789" (3 groups with 3 digits each, split by dashes). Now, I want to extend the class PhoneNumber with a static Parse and TryParse method. What is the best way to do so while keeping following requirements:

  • Code reuse between Parse and TryParse
  • Well-known API (see DateTime.TryParse)

Following code shows the first attempt of how I would implement such Parse/TryParse methods. TryParseInternal collects a list of exceptions/anomalies for the given codeline input string. Static method Parse throws an exception if anything is wrong with the given codeline while TryParse just returns true/false to indicate parsing success.

public class PhoneNumber
{
    public string Part1 { get; }
    public string Part2 { get; }
    public string Part3 { get; }

    public PhoneNumber(string part1, string part2, string part3)
    {
        Part1 = part1;
        Part2 = part2;
        Part3 = part3;
    }

    // Example implementation for TryParse
    public static bool TryParse(string codeline, out PhoneNumber value)
    {
        if (!TryParseInternal(codeline, out value).Any())
        {
            return true;
        }

        return false;
    }

    // Example implementation for Parse
    public static PhoneNumber Parse(string codeline)
    {
        var exceptions = TryParseInternal(codeline, out var esr);
        if (!exceptions.Any())
        {
            return esr;
        }

        throw new ParseException(exceptions);
    }

    private static List<Exception> TryParseInternal(string codeline, out PhoneNumber value)
    {
        var exceptions = new List<Exception>();
        if (codeline == null)
        {
            exceptions.Add(new ArgumentNullException(nameof(codeline)));
        }

        else if (codeline == "")
        {
            exceptions.Add(new ArgumentException(nameof(codeline)));
        }

        var match = Regex.Match(codeline, @"^(\d{3})-(\d{3})-(\d{3})$");
        if (match.Success == false)
        {
            exceptions.Add(new ArgumentException("Format is invalid", nameof(codeline)));
        }

        if (exceptions.Any())
        {
            value = null;
        }
        else
        {
            value = new PhoneNumber(match.Groups[1].Value, match.Groups[2].Value, match.Groups[3].Value);
        }

        return exceptions;
    }

    public class ParseException : AggregateException
    {
        public ParseException(string message) : base(message)
        {
        }

        public ParseException(IEnumerable<Exception> exceptions) : base(exceptions)
        {
        }
    }
}

Following XUnit tests demonstrate failing/succeeding Parse and TryParse scenarios:

public class PhoneNumberTests
{
    [Fact]
    public void ParseTest_Success()
    {
        // Arrange
        var codeline = "123-456-789";

        // Act
        var esr = PhoneNumber.Parse(codeline);

        // Assert
        esr.Part1.Should().Be("123");
        esr.Part2.Should().Be("456");
        esr.Part3.Should().Be("789");
    }

    [Fact]
    public void ParseTest_InvalidFormat()
    {
        // Arrange
        var codeline = "X-X-X";

        // Act
        Action action = () => PhoneNumber.Parse(codeline);

        // Assert
        action.Should().Throw<PhoneNumber.ParseException>().Which.InnerExceptions[0].Message.Should().Contain("Format is invalid");
    }

    [Fact]
    public void TryParseTest_Success()
    {
        // Arrange
        var codeline = "123-456-789";

        // Act
        var success = PhoneNumber.TryParse(codeline, out var esr);

        // Assert
        success.Should().BeTrue();
        esr.Part1.Should().Be("123");
        esr.Part2.Should().Be("456");
        esr.Part3.Should().Be("789");
    }

    [Fact]
    public void TryParseTest_InvalidFormat()
    {
        // Arrange
        var codeline = "X-X-X";

        // Act
        var success = PhoneNumber.TryParse(codeline, out var esr);

        // Assert
        success.Should().BeFalse();
        esr.Should().BeNull();
    }
}

Is there anything in the given approach you would do differently? Do you see any potential problems? What about performance? Would it be better to collect strings instead of exceptions in terms of memory overhead?

Any recommendation/hint is welcome.

Update 2019-02-01: Thanks for the discussion so far. The focus should not lay on the phone number and the 3x3 digits I chose. This format does not even exist in my home country. It's just about how you would implement Parse and TryParse as static methods of a class.

Update 2019-02-08: I found a proper solution for this, but I guess it's not relevant anymore as some people here are more concerned about collecting rewards than finding solutions. Thanks for those who contributed.

\$\endgroup\$

closed as off-topic by t3chb0t, Toby Speight, vnp, Gerrit0, Pieter Witvoet Feb 5 at 8:50

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Toby Speight, vnp, Gerrit0, Pieter Witvoet
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 5
    \$\begingroup\$ Be careful with \d in your regex. "๑๒๓-๔๕๖-๗๘๙" will be accepted as a valid phone number. Doesn't have to be a problem, but depending on what is done next with the data, it might just be ... [0-9] is probably closer to what you intended. \$\endgroup\$ – linac Jan 31 at 13:17
  • 4
    \$\begingroup\$ Phone numbers aren't amenable to parsing. Your entire premise is wrong. See github.com/googlei18n/libphonenumber/blob/master/FALSEHOODS.md \$\endgroup\$ – Roger Lipscombe Jan 31 at 15:09
  • 1
    \$\begingroup\$ Came here to share libphonenumber too. Parsing phone number is hard. Please avoid doing it for any serious work. Whenever you can, use libphonenumber. official C# port \$\endgroup\$ – aloisdg Jan 31 at 16:20
  • 2
    \$\begingroup\$ I vote to close this question as off-topic because OP posted code that has no purpose and thus lacks context, consequently isn't interested in a review but a discussion about the design of the two methods. OP should have asked it on Software Engineering. \$\endgroup\$ – t3chb0t Feb 1 at 9:00
  • 2
    \$\begingroup\$ found a proper solution for this, but I guess it's not relevant anymore as some people here are more concerned about collecting rewards than finding solutions. - this is not true - each community has its special rules and this one requires real and working code which you have not provided so consequently it got closed because reviewing and improving code that virtually does not exist is pointless. Would you have asked this question on Software Engineering where it perfectly fits their rules the outcome would have been quite different. \$\endgroup\$ – t3chb0t Feb 8 at 7:36
10
\$\begingroup\$
    public PhoneNumber(string part1, string part2, string part3)
    {
        Part1 = part1;
        Part2 = part2;
        Part3 = part3;
    }

I'm confused. The existence of TryParse tells me that there are important validity constraints on the number, but there's a public constructor which doesn't validate its arguments. It seems that I could easily make an instance of PhoneNumber for which Parse(phoneNumber.ToString()) doesn't round-trip.

In fact, I'm even more confused: PhoneNumber doesn't override ToString() or any of the identity methods (GetHashcode, Equals, operator==, etc). Why not?


    public static bool TryParse(string codeline, out PhoneNumber value)
    {
        if (!TryParseInternal(codeline, out value).Any())
        {
            return true;
        }

        return false;
    }

IMO this is a failure to properly use the Boolean type. I would prefer

    public static bool TryParse(string codeline, out PhoneNumber value)
    {
        return !TryParseInternal(codeline, out value).Any();
    }

    private static List<Exception> TryParseInternal(string codeline, out PhoneNumber value)
    {
        var exceptions = new List<Exception>();
        if (codeline == null)
        {
            exceptions.Add(new ArgumentNullException(nameof(codeline)));
        }

        else if (codeline == "")
        {
            exceptions.Add(new ArgumentException(nameof(codeline)));
        }

        var match = Regex.Match(codeline, @"^(\d{3})-(\d{3})-(\d{3})$");

This aggregation strategy lets you down (and so does your test suite) when I pass null for codeline. Instead of the ArgumentNullException or ParseException which I would expect, I get a NullReferenceException from Regex.Match.


Another issue I see with the aggregation strategy is that the code repeats itself a bit: TryParseInternal checks exceptions.Any() to decide how to handle the out parameter, and then its caller checks esr.Any() to decide how to handle the out parameter. If you are attached to the aggregation (I personally don't see the point) then this might be simpler if the inner method returned ParseException instead of List<Exception>.


Which prompts me to say: code to the interface, not the implementation. If a method can return IList<T> then that's preferable to returning List<T>.

\$\endgroup\$
  • \$\begingroup\$ Mhmm... the last statement is questionable as arrays are also of type IList<T>. I'd say it's better to use the interface as it gives you more options especially during testing. \$\endgroup\$ – t3chb0t Jan 31 at 11:41
  • \$\begingroup\$ @t3chb0t, I don't understand. What's the problem with using a type which arrays implement? And why do you say that the statement is questionable and then agree with it? \$\endgroup\$ – Peter Taylor Jan 31 at 11:57
  • 1
    \$\begingroup\$ Oh, crappy-crap, I misread the then that's preferable to returning as then it's preferable to return --- sorry!!! ;-] \$\endgroup\$ – t3chb0t Jan 31 at 12:00
  • 2
    \$\begingroup\$ @Peter I'm arguing that your claim "code to the interface, not the implementation" is wrong for some very prominent classes of the standard library due to errors in their implementation. It's a good idea in principle but shouldn't be stated too generally without mentioning the caveats. Seemed fitting considering that t3chb0t already brought it up unintentionally. \$\endgroup\$ – Voo Jan 31 at 16:02
  • 1
    \$\begingroup\$ @Voo since IList<T> implements the ICollection<T> interface and this one provides the IsReadOnly property it's perfectly suited for arrays too thus a list does not necessarily has to support Add, it's then a read-only-list/collection. \$\endgroup\$ – t3chb0t Jan 31 at 16:15
4
\$\begingroup\$

Class design

Apart from the parsing logic I find the PhoneNumber class should not have its three PartX properties but a single property Value that stores all digits.

How many parts a phone number has it's purely a visual representation and should be implemented either by the UI or alternatively by ToString(format).

Parsing

I wouldn't say that restricting phone numbers to exact three equally long parts is a good idea as there is no standard format for them. I sugesst to just match the digits and other allowed separators and capture only digts:

var digits =
    Regex
        .Matches("123-456-789", @"(?:(\d)|[-])")
        .Cast<Match>()
        .Where(m => m.Groups[1].Success)
        .Select(m => m.Value);
\$\endgroup\$
  • 1
    \$\begingroup\$ Worse case if he keeps 3 parts, they should be named properly such as AreaCode, Prefix, and LineNumber. \$\endgroup\$ – Rick Davin Jan 31 at 19:18
  • \$\begingroup\$ Agree. The question however was how Parse/TryParse can be implemented in a proper and safe way. The phone number example is just fictive. \$\endgroup\$ – user191754 Feb 8 at 7:07

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