8
\$\begingroup\$
try{save.Username = usernamedetails.Rows[0].ItemArray[0].ToString(); }
catch{ save.Username = ""; }
try { save.Firstname = dtdetails.Rows[0].ItemArray[1].ToString(); }
catch { save.Firstname = ""; }
try { save.LastName = dtdetails.Rows[0].ItemArray[2].ToString(); }
catch { save.LastName = ""; }
try { save.Address1 = dtdetails.Rows[0].ItemArray[3].ToString(); }
catch { save.Address1 = ""; }
try { save.Address2 = dtdetails.Rows[0].ItemArray[4].ToString(); }
catch { save.Address2 = ""; }
try { save.Postoffice = dtdetails.Rows[0].ItemArray[5].ToString(); }
catch { save.Postoffice = ""; }
try { save.District = dtdetails.Rows[0].ItemArray[6].ToString(); }
catch { save.District = ""; }
try{ save.State = dtdetails.Rows[0].ItemArray[7].ToString(); }
catch{ save.State = ""; }
try { save.Country = dtdetails.Rows[0].ItemArray[8].ToString(); }
catch { save.Country = ""; }
try { save.MobileNumber = dtdetails.Rows[0].ItemArray[9].ToString(); }
catch { save.MobileNumber = ""; }
try { save.LandLine = dtdetails.Rows[0].ItemArray[10].ToString(); }
catch { save.LandLine = ""; }
try { save.Pin = dtdetails.Rows[0].ItemArray[11].ToString(); }
catch { save.Pin = ""; }
try { save.Cmpnyname = dtdetails.Rows[0].ItemArray[12].ToString(); }
catch { save.Cmpnyname = ""; }
try { save.Design = dtdetails.Rows[0].ItemArray[13].ToString(); }
catch { save.Design = ""; }
try { save.Loc = dtdetails.Rows[0].ItemArray[14].ToString(); }
catch { save.Loc = ""; }
try { save.CmpnyPincode = dtdetails.Rows[0].ItemArray[15].ToString(); }
catch { save.CmpnyPincode = ""; }
try { save.EmailID = dtdetails.Rows[0].ItemArray[16].ToString(); }
catch { save.EmailID = ""; }
try { save.OffNumber = dtdetails.Rows[0].ItemArray[17].ToString(); }
catch { save.OffNumber = ""; }

Is this type of strategy suitable for coding?

\$\endgroup\$
  • 27
    \$\begingroup\$ YUCK! ​​​​​​ \$\endgroup\$ – Jeff Mercado May 17 '13 at 5:42
  • \$\begingroup\$ What is the reason for the catches? Are you worries that the column won't exist? That its value will be null? Something else? A combination of the above? \$\endgroup\$ – svick May 17 '13 at 10:01
  • \$\begingroup\$ @svick I'd guess both. Either catching null reference exceptions or index out of range exceptions - either way, my eyes are bleeding. \$\endgroup\$ – RobH May 17 '13 at 12:52
  • \$\begingroup\$ As @svick said, you really need to say why you're trying to catch line-by-line exceptions for us to be useful. \$\endgroup\$ – Bobson May 17 '13 at 14:07
  • \$\begingroup\$ My first reaction is that there is a better data object to use for the information than a DataTable. The only two cases I could think of off the top of my head why someone would use it is because they are either reading data out of a database (in which case an ORM would be advisable) or from a DataGridView (in which case data binding save or some view model representation of save would be preferable). \$\endgroup\$ – Dan Lyons May 23 '13 at 17:41
14
\$\begingroup\$
  1. You shouldn't use general catch, you should always catch only the exceptions that you actually want to catch.
  2. Avoidable exceptions like index out of range or null reference should indicate an error in your code. Instead of handling them, you should make sure they never happen.

For your specific case, you could write an extension method that checks that the index isn't out of range and that the value isn't null (and returns empty string if it is).

Something like:

public static string GetAsStringOrEmpty(this object[] array, int index)
{
    if (array == null)
        throw new ArgumentNullException("array");

    if (index >= 0 && index < array.Length)
    {
        string value = array[index] as string;

        if (value != null)
            return value;
    }

    return string.Empty;
}

You would then use it like this:

var array = dtdetails.Rows[0].ItemArray;
save.Firstname = array.GetAsStringOrEmpty(1);
save.LastName = array.GetAsStringOrEmpty(2);
…
|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

First: Never have an exception raised just because you're too lazy to validate the input. Second: The very repetitive wording begs for some improvement. You're dealing with the same objects in each line pair, so why not implement some array and null checks in sub functions?

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

I suggest the use of the ternary Operator like this:

 String details=(usernamedetails.Rows[0]!=null)?usernamedetails.Rows[0].ItemArray[0] as String:null;
 save.Username=(details!=null)?details.toString():"";

So you prevent the exceptions.

And besides your code would be much more readable, than a jungle of try-catch-blocks.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ ItemArray is an Object[] so you'd need to cast usernamedetails.Rows[0].ItemArray[0] to a string. \$\endgroup\$ – RobH May 17 '13 at 9:55
  • 1
    \$\begingroup\$ This will still fail if Rows is empty. It's also the perfect use for ?? \$\endgroup\$ – Bobson May 17 '13 at 14:03
1
\$\begingroup\$

Convert the repeated calls to a function, and remove the generalised catch.

// Assuming DataTable containing the source list
function string GetValue(DataTable dt, int idx)
{
   // Check if table is not null and contains data
   if (dt == null || dt.Rows.Count == 0) return String.Empty;
   // Check if index exists
   if (dt.Rows[0].ItemArray.Count <= idx && idx >= 0) return String.Empty;
   // Check if the value is null
   if (dt.Rows[0].ItemArray[idx] == null) return String.Empty;

   return dt.Rows[0].ItemArray[idx].ToString();
}

You can then call the sub function

save.Firstname = GetValue(dtdetails, 1);

And repeat for the various values.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ dt.Rows[0].ItemArray is used several times in your code. Why not store that value in a variable? E.g. var itemArray = dt.Rows[0].ItemArray \$\endgroup\$ – GiddyUpHorsey May 27 '13 at 0:21
0
\$\begingroup\$

Quoting @svick's answer:

Avoidable exceptions like index out of range or null reference should indicate an error in your code. Instead of handling them, you should make sure they never happen.

Seems to me that the try/catching as shown is just a substitute for checking for null. Potentially there are casting issues, but that would be real "rouge data" and a true Exceptional situation. Ensure valid/desired/non-null data in your datarow at the time it's populated. Better use of the .NET System.Data classes & properties will go a long way!

Note the potential exceptions thrown by ItemArray. Work with them.

DataRow newRow = new DataRow();
newRow.AllowDBNull = false;   // or "true". As desired.
newrow.DefaultValue = string.Empty;

// get the Object[] to populate the row.

// The Dataxxxx classes have mechanisms & properties for handling, in particular, 
// null values relatively seamlessly when coming/going to a database. This code is
// a hint of that kind of capability

try {
    newrow.ItemArray = dataObjectArray;  // any null values become string.Empty
}catch (NoNullAllowedException e) {
    // "this will never happen!". But if it does, we'll know it explicitly
}catch(ArgumentException e) { 
    // we know column count and array length do not match.
}catch(Exception e) {
   // I really cared about the 2 specific ones above. The rest we'll get here.
}

// now we can go through all the `ItemArray[x].ToString` and not worry about null.

try {
    // all the ToString-ing. All in this one try block.
    // at this point an exception truly is exceptional
}catch(InvalidCastException e) {}
catch (Exception e) {}
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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