TryParse pattern / Non Default constructor with readonly fields

Question

How do people generally implement a parsing constructor with the TryParse pattern, when they have readonly backing fields, and a non default constructor, that would normally do the parsing?

Below is a contrived example of what I'm talking about, and the pattern I've settled on, but it seems clunky. In reality some of the types have a large number of properties. Sure, I can create a method that would take n ref arguments, do the parsing, and wire them up that way, but having a method with 15 arguments in some cases is a pain/smelly also.

The idea of two constructors, plus having to copy the result of the try parse to the readonly fields in the parsing constructor smells a little.

Anyone else have a better pattern?

Edit: Provide some more context

What I'm attempting to do, is refactor a large(ish) codebase, which has many types like the example below, where there is parsing of string arguments supplied to the constructor. Right now, all of the code uses constructor parsing, and the logic for this parsing is all done in the constructor. Messy and troublesome.

First thing I want to do, is move this code out of the constructor, into a factory method, (TryParse), but preserve the constructor signature, so I dont have lots of changes to the codebase. Long term, something better can be done when there is time.

Currently, the difficulty is keeping the constructure signature intact for the existing code, while allowing new code to use the TryParse pattern, and keep the readonly fields. If I relaxed the readonly fields, the entire process would be easier, but I'd rather not.

public class Point
{
   private readonly float x, y;
   public float X { get { return x; } }
   public float Y { get { return y; } }

   public Point(string s)
   {
      Point result;

      if (TryParse(s, out result))
      {
         this.x = result.x;
         this.y = result.y;
      }
      else
      {
         throw new System.ArgumentException("cant parse");
      }
   }

   private Point(float x,float y) // for the sake of the example, this wouldnt have any use as public
   {
      this.x = x;
      this.y = y;
   }

   public static bool TryParse(string s,out Point result)
   {
      var numbers = s.Split(',');

      if(numbers.Length == 2)
      {
         result = new Point(float.Parse(numbers[0]),float.Parse(numbers[0]));
         return true;
      }
      else
      {
         result = null;
         return false;
      }
   }
}

Answer 1

Your current approach doesn't really work - because float.Parse can throw an exception. I'd use something like:

public static bool TryParse(string s, out Point result)
{
   var numbers = s.Split(',');
   if (numbers.Length == 2)
   {
       float x, y;
       if (float.TryParse(numbers[0], out x) && 
           float.TryParse(numbers[1], out y))
       {
           result = new Point(x, y);
           return true;  
       }    
   }
   result = null;
   return false;
}

As StriplingWarrior says, I'd get rid of the parsing constructor to start with - if you're using TryParse anyway, add a Parse method too.

The idea of two constructors, plus having to copy the result of the try parse to the readonly fields in the parsing constructor smells a little.

Well with this approach, you only need one constructor - but what's wrong with passing values to a constructor and it copying them into the object? That seems natural to me.

Alternatively, you could use the approach we use in Noda Time where you create a whole separate object responsible for parsing and formatting, and a ParseResult<T> type which is able to represent the success/failure of a parse, while retaining the ability to throw an exception with meaningful information about the failure. Personally I find it a lot nicer than the BCL pattern, although arguably I'm biased :)

Answer 2

I usually try to avoid parsing constructors. There is a popular sentiment that constructors should do as little as possible, and usually only take arguments that get stored directly into fields for later use. If you want to parse a string, that's not the responsibility of the constructor.

You have a TryParse method that follows the typical expected pattern for C#/.NET apps. Make the changes that Jon Skeet recommends, then expect your users to call that method directly if they want to parse a string to a Point.