CRUD code for a finance WebForm

Question

I have a finance web-application in ASP.NET WebForms using Telerik RAD controls. Two event handlers wound up being very similar in implementation.

What steps could I take to improve the organization of and/or the efficiency or DRY-ness of the code? What other issues / code-smells are present?

The Code

---

• Update Event

      protected void gv_FinancePositionList_UpdateCommand(object sender, GridCommandEventArgs e)
              {
                  var item = (GridEditFormItem) e.Item;
                  int financeListId = int.Parse(item.GetDataKeyValue(keyName: "FinanceListId").ToString());
                  int companyId = int.Parse(((RadComboBox) item.FindControl(id: "ddl_Company")).SelectedValue);
                  int year = ((RadMonthYearPicker) item.FindControl(id: "pick_Year")).SelectedDate.Value.Year;
                  int periodTypeId = int.Parse(((RadioButtonList) item.FindControl(id: "rbtn_PeriodTypeName")).SelectedValue);
                  int partition = 1;
                  if (((RadDropDownList) item.FindControl(id: "ddl_PeriodType")).Visible)
                  {
                      partition = int.Parse(((RadDropDownList) item.FindControl(id: "ddl_PeriodType")).SelectedValue);
                  }
                  string cashAndEquivalents = ((RadNumericTextBox) item.FindControl(id: "txt_CashAndEquivalents")).Text;
  
                  decimal? cashAndEquivalentsVal = null;
                  if (!string.IsNullOrEmpty(cashAndEquivalents))
                  {
                      cashAndEquivalentsVal = decimal.Parse(cashAndEquivalents);
                  }
  
                  string shortTermInvestments = ((RadNumericTextBox) item.FindControl(id: "txt_ShortTermInvestments")).Text;
  
                  decimal? shortTermInvestmentsVal = null;
                  if (!string.IsNullOrEmpty(shortTermInvestments))
                  {
                      shortTermInvestmentsVal = decimal.Parse(shortTermInvestments);
                  }
   using (var ctx = new Model())
              {
       FinancePositionList financePositionList =     ctx.FinancePositionLists.Find(financeListId);
                  financePositionList.CompanyId = companyId;
                  financePositionList.CashAndEquivalents = cashAndEquivalentsVal;
  financePositionList.ShortTermInvestments = shortTermInvestmentsVal;
                 ctx.SaveChanges();
                }
  }
  

• Insert event

  protected void gv_FinancePositionList_InsertCommand(object sender, GridCommandEventArgs e)
          {
              var item = (GridEditFormItem) e.Item;
  
              int companyId = int.Parse(((RadComboBox) item.FindControl(id: "ddl_Company")).SelectedValue);
              int year = ((RadMonthYearPicker) item.FindControl(id: "pick_Year")).SelectedDate.Value.Year;
              int periodTypeId = int.Parse(((RadioButtonList) item.FindControl(id: "rbtn_PeriodTypeName")).SelectedValue);
              int partition = 1;
              if (((RadDropDownList) item.FindControl(id: "ddl_PeriodType")).Visible)
              {
                  partition = int.Parse(((RadDropDownList) item.FindControl(id: "ddl_PeriodType")).SelectedValue);
              }
              string cashAndEquivalents = ((RadNumericTextBox) item.FindControl(id: "txt_CashAndEquivalents")).Text;
  
              decimal? cashAndEquivalentsVal = null;
              if (!string.IsNullOrEmpty(cashAndEquivalents))
              {
                  cashAndEquivalentsVal = decimal.Parse(cashAndEquivalents);
              }
  
              string shortTermInvestments = ((RadNumericTextBox) item.FindControl(id: "txt_ShortTermInvestments")).Text;
  
              decimal? shortTermInvestmentsVal = null;
              if (!string.IsNullOrEmpty(shortTermInvestments))
              {
                  shortTermInvestmentsVal = decimal.Parse(shortTermInvestments);
  
     var financePositionList = new FinancePositionList
          {
              CompanyId = companyId,
               CashAndEquivalents = cashAndEquivalentsVal,
               ShortTermInvestments = shortTermInvestmentsVal
           }
  
    using (var ctx = new Model())
          {
  
              ctx.FinancePositionLists.Add(financePositionList);
              ctx.SaveChanges();
  
          }
       } 
  

Answer 1

• Well. The first thing you need to do is seperate the business logic and the repository from the UI. You are currently linking the logic to the UI, which is not considered best practice. You will have to rewrite a whole UI if you want to move to web. You will also do the same for Repository in case you wanted to change to different store.

• Use interfaces, which will help you a lot with abstracting and testing.

• How do you handle exceptions (from repository and from parsing)? You probably need something like that.

Answer 2

I would suggest:

1. Removing dead code (some unused variables)
2. Creating a class that can be reused to contain 'Financial details'
3. Creating some methods to reuse code ('getFinancialDetails' and 'getFinancialDetailsValue' in my example below)
4. Using something different than 'item.FindControl' and 'control Ids' as it links your logic to your UI.
5. Adding more exception handling for data from your UI if needed

Here's an example (I'm unsure if it's 100% working):

private class FinancialDetails
{
    public int CompanyId {get; set;}
    public decimal? CashAndEquivalentsVal {get; set;}
    public decimal? ShortTermInvestmentsVal {get; set;}
}

private FinancePositionList financePositionListMapper(FinancePositionList financePositionList, FinancialDetails financialDetails)
{
    financePositionList.CompanyId = financialDetails.CompanyId;
    financePositionList.CashAndEquivalentsVal = financialDetails.CashAndEquivalentsVal;
    financePositionList.ShortTermInvestmentsVal = financialDetails.ShortTermInvestmentsVal;
    return financePositionList;
}

protected void gv_FinancePositionList_UpdateCommand(object sender, GridCommandEventArgs e)
{
    FinancialDetails financialDetails = getFinancialDetails((GridEditFormItem) e.Item);
    int financeListId = int.Parse(((GridEditFormItem) e.Item).GetDataKeyValue(keyName: "FinanceListId").ToString());
    using (var ctx = new Model())
    {
        FinancePositionList financePositionList = ctx.FinancePositionLists.Find(financeListId);     
        financePositionList.CompanyId = financialDetails.CompanyId;
        financePositionList.CashAndEquivalents = financialDetails.CashAndEquivalentsVal;
        financePositionList.ShortTermInvestments = financialDetails.ShortTermInvestmentsVal;
        ctx.SaveChanges();
    }
}

protected void gv_FinancePositionList_InsertCommand(object sender, GridCommandEventArgs e)
{
    FinancePositionList financePositionList = financePositionListMapper(new FinancePositionList(), getFinancialDetails((GridEditFormItem) e.Item));
    using (var ctx = new Model())
    {
        ctx.FinancePositionLists.Add(financePositionList);
        ctx.SaveChanges();
    }
}

private FinancialDetails getFinancialDetails(GridEditFormItem item)
{
    return FinancialDetails financialDetails = new FinancialDetails
    {
        CompanyId = int.Parse(((RadComboBox) item.FindControl(id: "ddl_Company")).SelectedValue),
        CashAndEquivalentsVal = getFinancialDetailsValue(item, "txt_CashAndEquivalents"),
        ShortTermInvestmentsVal = getFinancialDetailsValue(item, "txt_ShortTermInvestments")
    }
}

private decimal? getFinancialDetailsValue(GridEditFormItem item, string controlId)
{
    string value = ((RadNumericTextBox) item.FindControl(id: controlId)).Text;
    decimal? financialDetailsValue = null;
    if (!string.IsNullOrEmpty(value))
    {
        financialDetailsValue = decimal.Parse(value);
    }
    return financialDetailsValue;
}

More work could be done to unlink your UI from your logic.