views:

426

answers:

3

I'm currently refactoring code to replace Convert.To's to TryParse.

I've come across the following bit of code which is creating and assigning a property to an object.

List<Person> list = new List<Person>();

foreach (DataRow row in dt.Rows)
{
     var p = new Person{ RecordID = Convert.ToInt32(row["ContactID"]) };

     list.Add(p);
}

What I've come up with as a replacement is:

var p = new Person { RecordID = Int32.TryParse(row["ContactID"].ToString(), out RecordID) ? RecordID : RecordID };

Any thoughts, opinions, alternatives to what I've done?

+4  A: 

Write an extension method.

public static Int32? ParseInt32(this string str) {
    Int32 k;
    if(Int32.TryParse(str, out k))
        return k;
    return null;
}
Justice
Wouldn't you want it to return 0 instead of null if false to be consistent with the original functionality?
Godless667
In this case, because there should be a difference between failing to parse (null is returned) and paring 0 (0 is returned).
xsl
@Godless No. You would want something generic which is thus reusable. If you need to default to 0, then you would use ("1234".TryParseInt32() ?? 0).
Justice
This is also not readable. One would expect a method named "TryParseInt32" returns bool.
Serhat Özgel
Renamed the extension method.
Justice
+1  A: 

I'd use an alternative implementation TryParse which returns an int?:

public static int? TryParseInt32(string x)
{
    int value;
    return int.TryParse(x, out value) ? value : (int?) null;
}

Then you can write:

var p = new Person { RecordID = Helpers.TryParseInt32(row["ContactID"].ToString()) ?? 0 };

(Or use a different default value, if you want - either way it'll be visible in your code.)

Jon Skeet
A: 

I suggest separate the TryParse part from initializer. It will be more readable.

int recordId;
Int32.TryParse(row["ContactID"].ToString(), out recordID)

foreach (DataRow row in dt.Rows)
{
     var p = new Person{ RecordID = recordId };
     list.Add(p);
}
yapiskan
I thought of that as well (though you would need to put that part within the foreach block).I was trying to go for terser code. Especially if you are setting multiple properties while creating the object.
Godless667
So why don't you create a constructor for Person that gets a DataRow and do what you want in it?
yapiskan
Why on earth should a Person class need to understand the semantics of relational-database persistence via ODBC and ADO.NET? Should it need to understand how to construct itself from a binary network stream? From CSV files? etc?
Justice
Exactly what Justice said ;)
Godless667
@Justice - Sure, you are right. I have totally nothing to say :)
yapiskan