Nested Try/Catch Blocks

Question

I'm allowing users to upload data from an Excel file to my database, and want to display friendly error messages if one of their cells is in the wrong format. In total, there will be 20 or so cells to upload, but right now I'm just trying two. The Excel upload I'm trying has some random text in the MedicalTotal field, which should instead be formatted as a decimal. I verified that my code is looking at the right field. The following controller code does nothing. Nothing is written to the database, and the same view is reloaded.

In the watch window, I get a message of "Convert.ToDecimal(table[0]) threw an exception of type System.FormatException" I have tried the code with both Exception and FormatException. I'm open to other methods of catching errors, keeping in mind that I'll be repeating the try/catch process about 20 times.

[Authorize]
[HttpPost]
public ActionResult CreateBenefitSummary(HttpPostedFileBase FileUpload, Guid ResponseId)
{
    BenefitsUploadViewModel model = new BenefitsUploadViewModel();

if (FileUpload != null)
{
    // tdata.ExecuteCommand("truncate table OtherCompanyAssets");  
    if (FileUpload.ContentType == "application/vnd.ms-excel" || FileUpload.ContentType == "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet")
    {

        string filename = FileUpload.FileName;
        string targetpath = Server.MapPath("~/Doc/");
        FileUpload.SaveAs(targetpath + filename);
        string pathToExcelFile = targetpath + filename;
        var connectionString = "";
        if (filename.EndsWith(".xls"))
        {
            connectionString = string.Format("Provider=Microsoft.Jet.OLEDB.4.0; data source={0}; Extended Properties=Excel 8.0;", pathToExcelFile);
        }
        else if (filename.EndsWith(".xlsx"))
        {
            connectionString = string.Format("Provider=Microsoft.ACE.OLEDB.12.0;Data Source={0};Extended Properties=\"Excel 12.0 Xml;HDR=YES;IMEX=1\";", pathToExcelFile);
        }

        var excelFile = new ExcelQueryFactory(pathToExcelFile);

        var table = (from a in excelFile.WorksheetRangeNoHeader("B2", "B32", "Benefits Template") select a[0]).ToList();

        TempData["ResponseId"] = ResponseId;
        try
        {
            Benefits b = new Benefits();
            b.ResponseId = ResponseId;
            try
            {
                b.MedicalTotal = Convert.ToDecimal(table[0]);
            }
            catch (FormatException)
            {
                model.ErrorList.Add("Medical Total cell must use Number, Currency, or Accounting format.");
            }
            b.StdSicknessAccident = Convert.ToDecimal(table[1]);
            if (model.ErrorList.Count > 0) {
                return View(model);
            }
            db.benefits.Add(b);
            db.SaveChanges();
        }

        catch (SqlException ex)
        {
            ViewBag.Message = ex.Message;
        }
        catch (Exception ex)
        {
            ViewBag.Message = ex.Message;
        }

        //deleting excel file from folder  
        if ((System.IO.File.Exists(pathToExcelFile)))
        {
            System.IO.File.Delete(pathToExcelFile);
        }
        TempData["ResponseId"] = ResponseId;
        return RedirectToAction("CreateBenefitSummary", "Surveys");

    }

}
return View();

}

You might want to use [Decimal.TryParse](https://learn.microsoft.com/nl-nl/dotnet/api/system.decimal.tryparse?view=netframework-4.7) instead of convert — jao, Jan 15 '19 at 17:07
Also, I would suggest that you invert your if statements so you can prevent nested if's, which makes the code more readable (fail early). So instead of `if (FileUpload != null)` you'd use `if (FileUpload == null) { return View(); } ` — jao, Jan 15 '19 at 17:09

Neil N · Accepted Answer · 2019-01-15T18:31:05.723

2

Your convert.ToDecimal should probably use a Decimal.TryParse instead, then showing the error message after an if statement seeing if the results parsed. Using try/catch for flow control is generally considered bad practice.

something like:

Decimal decVal;
if (Decimal.TryParse(table[0], out decVal))
{
     b.MedicalTotal = decVal;
}
else
{
     model.ErrorList.Add("Medical Total cell must use Number, Currency, or Accounting format.");
}

edited Jan 15 '19 at 18:31

answered Jan 15 '19 at 17:06

Neil N

24,862
16
85
145

Perfect! Thank you – extensionhelp Jan 15 '19 at 17:29
1

That's missing the `out` for the second parameter and the logic is backwards as the if is the successful path but it setting the error. – juharr Jan 15 '19 at 17:38
You can now avoid declaring decVal up front and inline the out variable declaration if using C# 7+. https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-7#out-variables – fourwhey Jan 15 '19 at 18:43

Nested Try/Catch Blocks

1 Answers1

Linked