mjhjmtu

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, @"^(d3)-(d3)-(d3)$");
 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.

 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, @"^(d3)-(d3)-(d3)$");

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>.

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);