C# Refactoring: Parsing comma-seperated string[] arg

Okay, let's say with have this:

string productUidsPostValue = 
    "693C850B-2B0B-4429-98F8-AE99E92991A8,F37858BD-22E5-4077-BADD-9AFCDCC92628";

I want to turn this into a List the easiest way possible. And of course, the strings in productUidsPostValue need to be strongly typed as Guids if they are valid Guids. This is the code I have written. Surely it can be refactored or reduced, right?

if (string.IsNullOrEmpty(productUidsPostValue))
{
    throw new InvalidOperationException
        ("this.Request.Form['CheckoutProductUids'] cannot be null or empty.");
}

var seperatedUids = productUidsPostValue.Split(',');

var productUids = new List<Guid>(seperatedUids.Length);

Guid guid;

foreach (var productUid in seperatedUids)
{
      if (!Gu开发者_如何学GoidHelper.TryParse(productUid, out guid))
      {
            productUids.Add(guid);
      }
}

This is the shortest code , i can think of , provided the productUidsPostValue is correctly formed(Guid in the correct format).

  string productUidsPostValue = "693C850B-2B0B-4429-98F8-AE99E92991A8,F37858BD-22E5-4077-BADD-9AFCDCC92628";
  List<Guid> seperatedUids = (from guid in productUidsPostValue.Split(',') select new Guid(guid)).ToList();

You could do something like this:

return productUidsPostValue.Split(',')
    .Where(productUid => { Guid tmp; return GuidHelper.TryParse(productUid, out tmp); })
    .Select(validProductUid => new Guid(validProductUid))
    .ToList();

Which is more literate, but I don't like the smell of the try parse method in there.

Personally, I'd try to refactor some of your GuidHelper.TryParse code out to another extension method, like:

public static Guid? ParseToNullableGuid(this string stringToParse)
{
    Guid? val = null;

    if(String.IsNullOrEmpty(stringToParse))
        return val;

    var guidPattern = @"[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{8}";
    var validGuid = new Regex(guidPattern, RegexOptions.Compiled);

    if (!validGuid.Match(stringToParse).Success)
        return val;

    try
    {
         val = new Guid(stringToParse);
    }
    catch(FormatException) { }

    return val;
}

Then you could do something more like:

return productUidsPostValue.Split(',')
    .Select(uid => uid.ParseToNullableGuid())
    .Where(uid => uid.HasValue)
    .Select(uid => uid.Value)
    .ToList();

For bonus points, you can do some more tests in the extension method, such as do string.indexof tests, to save the exception based programming, at least a little more than this already does.

This is the shortest I could do without turning into unreadable code. ps. The original code failed to check if TryParse was successful, and also it threw the wrong kind of exception.

if (string.IsNullOrEmpty(productUidsPostValue))
    throw new ArgumentNullException("this.Request.Form['CheckoutProductUids']");

foreach (var productUid in productUidsPostValue.Split(','))
{
    Guid guid;
    if (GuidHelper.TryParse(productUid, out guid))
        productUids.Add(guid);
}

You could use LINQ:

Guid guid;
List<Guid> productUids = productUidsPostValue.Split(',')
    .Select(s => {guid = Guid.Empty; GuidHelper.TryParse(s, out guid); return guid;});

Looks pretty clean (n simple) to me. Do you code-smell something ?

You could make the code a bit more concise if you figure out the C# equivalent Ruby's collect (I've omitted the guid validation part although you need it)

guidStrings.split(',').collect{|each_guid|  
   Guid.new(each_guid)
}

If you're using .NET 4.0 you can use the new Guid.TryParse. Otherwise, use your GuidHelper.TryParse instead in the code below.

string productUidsPostValue = "693C850B-2B0B-4429-98F8-AE99E92991A8,F37858BD-22E5-4077-BADD-9AFCDCC92628,F37858BD-22E5-4077-BADD-9AFCDCC9262-XYZ";
var query = productUidsPostValue.Split(',')
                .Select(s => {
                    Guid result;
                    return Guid.TryParse(s, out result) ? 
                                                (Guid?)result : (Guid?)null;
                })
                .Where(g => g.HasValue)
                .Select(g => g.Value)
                .ToList();

This comma separated string is unlikely a human input, so I would not recommend gracefully handling separate wrong guid entries in comma separated list.

If you put too much effort into safely handling wrong input here, you are actually looking for problems. For example, a client code could provide wrong input string, and you gracefully treat it as good input, but with different meaning.

Instead, I would recommend failing with any exceptions coming from guid parsing constructors for the entire string at once, if any of the items could not be parsed, so you can come up with the cleanest code possible.