ASP.NET MVC to produce a repair invoice

Question

I just started working with ASP.NET MVC a few weeks ago, and I'm finding that it can be very easy to write spaghetti code in the controllers. For my first project, I created a very simple view with a few controls. At first, all of my code was in the Index() action result. That worked okay for a while, but as more features get added to the page, the more the code grows. I would like my code to be split up into multiple action results. I made an attempt at refactoring.

Here is my View:

@model TpgInternalSite.Models.RepairInvoice

@{
    ViewBag.Title = "Repair Invoicing";
    Layout = "~/Views/Shared/_Layout100.cshtml";
}

<h2>Repair Invoice</h2>

<script type="text/javascript">
    $(document).ready(function () {

        $('@(ViewBag.SetFocusTo)').focus();

        $('#RmaNumber, #SerialNumber').keydown(function (event) {
            if (event.keyCode == 13 || event.keyCode == 9) {

                var RmaNumber = $('#RmaNumber').val();
                var SerialNumber = $('#SerialNumber').val();

                event.preventDefault(); //Stops enter key from triggering the Process button.  
                if (event.target.id == 'RmaNumber'){

                    var link = '/Invoice/RmaNumberScan?rmaNumber=' + RmaNumber;
                    location.href = link;
                }
                else {

                    var link = '/Invoice/SerialNumberScan?rmaNumber=' + RmaNumber + '&serialNumber=' + SerialNumber;
                    location.href = link;
                }
            }
        });
    });
</script>


<br />
<br />


<div class="form-horizontal">

    @using (Html.BeginForm("Index", "Invoice", FormMethod.Post))
    {
        <div class="control-group">
            <div class="control-label">
                RMA#
            </div>
            <div class="controls">
                @Html.TextBoxFor(x => x.RmaNumber)
            </div>
        </div>
        <div class="control-group">
            <div class="control-label">
                SERIAL#
            </div>
            <div class="controls">
                @Html.TextBoxFor(x => x.SerialNumber)
            </div>
        </div>
        <div class="control-group">
            <div class="control-label">
                Terminal Type:
            </div>
            <div class="controls">
                @Html.LabelForModel(Model.TerminalType)
            </div>
        </div>
        <div class="control-group">
            <div class="control-label">
                Warranty:
            </div>
            <div class="controls">
               @Html.CheckBox("warranty", Model.UnderWarranty, new { disabled = "disabled" })
            </div>
        </div>
        <div class="control-group">
            <div class="control-label">
                Repair Level:
            </div>
            <div class="controls">
                @Html.DropDownListFor(x => x.RepairLevels, new SelectList(Model.RepairLevels))
            </div>
        </div>
        <div class="form-actions">

           <input name="process" class="btn primary" type="submit" id="process" value="Process" />

        </div>
    }
    </div>
</>

And my controller looks like this:

    public ActionResult Index()
    {
        ViewBag.SetFocusTo = "#RmaNumber";
        Session["RmaDetail"] = null;
        return View(repairInvoice);
    }

    [HttpPost]
    public ActionResult Index(RepairInvoice ri)
    {
        if (Session["RmaDetail"] != null)
        {
            var rmaDetail = Session["RmaDetail"] as SVC05200;
            RepairInvoiceDataLayer.UpdateRmaRepairLevel(rmaDetail, ri.RepairLevels[0].Trim());
            ViewBag.SuccessMessage = "Repair level updated successfully.";
            ViewBag.SetFocusTo = "#RmaNumber";
            ModelState.Clear();
            ri.SerialNumber = string.Empty;
            Session["RmaDetail"] = null;
        }

        return View(ri);
    }


    public ActionResult RmaNumberScan(string rmaNumber)
    {
        repairInvoice.RmaNumber = rmaNumber;

        if (!string.IsNullOrEmpty(rmaNumber))
        {
            try
            {
                bool IsValidRma = RepairInvoiceDataLayer.IsValidRmaNumber(rmaNumber);

                if (IsValidRma)
                {
                    ViewBag.SetFocusTo = "#SerialNumber";
                }
                else
                {
                    ViewBag.AlertMessage = "RMA Number not found.";
                }
            }
            catch (Exception ex)
            {
                ViewBag.ErrorMessage = "An error occured searching for RMA Number.  Please try again.";
                log.Fatal(ex.ExceptionToString());
            }
        }

        return View("Index", repairInvoice);
    }


  public ActionResult SerialNumberScan(string rmaNumber, string serialNumber)
    {
        if (!string.IsNullOrEmpty(rmaNumber) && !string.IsNullOrEmpty(serialNumber))
        {
            try
            {
                if (serialNumber.Length > 20)
                {
                    serialNumber = serialNumber.Remove(20);
                }

                ModelState["SerialNumber"].Value = new ValueProviderResult(serialNumber, serialNumber, CultureInfo.CurrentCulture);

                var result = RepairInvoiceDataLayer.SelectRmaDetail(rmaNumber, serialNumber);

                if (result != null)
                {
                    if (result.Received == 1)
                    {
                        Nullable<bool> workOrderClosed = RepairInvoiceDataLayer.WorkOrderClosed(result.RETDOCID, result.LNSEQNBR, serialNumber);
                        if (workOrderClosed.HasValue)
                        {
                            if (workOrderClosed.Value)
                            {
                                Session["RmaDetail"] = result;
                                repairInvoice.TerminalType = result.ITEMNMBR.Trim();
                                repairInvoice.UnderWarranty = RepairInvoiceDataLayer.UnderWarranty(result.RETDOCID, result.LNSEQNBR, serialNumber, result.CUSTNMBR);
                                ViewBag.SetFocusTo = "#RepairLevels";
                            }
                            else
                            {
                                ViewBag.AlertMessage = "Work order not closed.";
                            }
                        }
                        else
                        {
                            ViewBag.AlertMessage = "Work order not found.";
                        }
                    }
                    else
                    {
                        ViewBag.AlertMessage = "Line item has not been received.";
                    }
                }
                else
                {
                    ViewBag.AlertMessage = "Serial Number not found.";
                }
            }
            catch (Exception ex)
            {
                ViewBag.ErrorMessage = "An error occured searching for Serial Number.  Please try again.";
                log.Fatal(string.Format("An error occured searching for Serial Number.  RMA Number: {0}  Serial Number: {1}", rmaNumber, serialNumber, ex));
            }
        }

        return View("Index", repairInvoice);
    }

Basically my page is setup to handle this current process flow:

1. User types a number into the RMA number textbox and hits enter or tab. JavaScript detects enter or tab key and navigates to /Invoice/RmaNumberScan and passes a query string with the number entered. Logic is performed in the RmaNumberScan action result.

2. Next, the user types a number into the serial scan textbox and hits tab or enter. The /Invoice/SerialNumberScan is called and passes query strings to the SerialNumberScan method.

3. User clicks the process button and then the Index() with the HTTPPOST method fires.

Am I on the right track, or am I doing MVC incorrectly? I come from a webforms background, and I want to split up my code. My attempt just feels so dirty and I don't like the fact that my action result name appears in the URL. I just want my code to be easily maintained and readable.

Since all of my database logic is done in my RepairInvoiceDataLayer, I still don't understand how the service layer comes into play here. What code from this controller would go into the service layer?

Answer 1

It sounds like you need to add a service layer where you can include all your business logic. This way your controller classes do not become bloated with business logic and they are only dealing with the handling of requests and the population of view-models.

Using thin controllers like this you can separate your logic out to different services and make it easier to practice the Single Responsibility Principle.

Please see this question and accepted answer but I've included an example below: Creating a service layer for my MVC application

Your controller:

[Dependency]
public IInvoiceService InvoiceService;

public ActionResult Index()
{
    //code
    return View(repairInvoice);
}

[HttpPost]
public ActionResult Index(RepairInvoice ri)
{ 
    this.InvoiceService.ProcessInvoice(ri.RmaNumber, ri.SerialNumber); // add on other params as appropriate
    return View(ri);
}

Service layer would consist of multiple classes, each with their own responsibility such as:

public class InvoiceService : IInvoiceService
{
    public void ProcessInvoice(string rmaNumber, string serialNumber) 
    {
        // Do any processing here
    }
}

I would recommend creating an interface for each separate service and then you can use Dependency Injection to instantiate the concrete classes using Unity or similar.

Answer 2

Although @SilverlightFox is right in that you need to separate your business logic from contoller; this in itself will not improve readability of your code; as the biggest problem, namely Arrow Code will still remain.

Arrow Code results from to many nesting levels in code. Also in an if/else statement handling the exceptional case later separates the condition from its consequence. By handling the exceptional case next to the condition and returning early solves both problems.

Your business logic method would look like this after said transformation.

public RmaDetail SerialNumberScan(int rmaNumber, int serialNumber, RepairInvoice repairInvoice) {
    var result = SelectRmaDetail(rmaNumber, serialNumber);

    if (result != null) throw new BusinessException("Serial Number not found.");

    if (result.Received != 1) throw new BusinessException("Line item has not been received.");

    bool? workOrderClosed = WorkOrderClosed(result.LNSEQNBR, result.LNSEQNBR, serialNumber);

    if (!workOrderClosed.HasValue) throw new BusinessException("Work order not found.");

    if (workOrderClosed.Value == false) throw new BusinessException("Work order not closed.");

    repairInvoice.TerminalType = result.ITEMNMBR.Trim();
    repairInvoice.UnderWarranty = RepairInvoiceDataLayer.UnderWarranty(result.RETDOCID, result.LNSEQNBR, serialNumber, result.CUSTNMBR);

    return result;
}

The rest of the spaghetti comes from user input validation. Although it, too, can be remedied by short-cut returns; the better way would be doing as much validation declaratively as possible. General error handling should also be done declaratively. After making those changes your SerialNumberScan action read like the following:

public ActionResult SerialNumberScan(string rmaNumber, string serialNumber)
{
    if (ModelState.IsValid)
    {
        ModelState["SerialNumber"].Value = new ValueProviderResult(serialNumber, serialNumber, CultureInfo.CurrentCulture);
        try
        {
            var rmaDetail = BusinessLayer.SerialNumberScan(rmaNumber, serialNumber, repairInvoice);
            Session["RmaDetail"] = rmaDetail;
            ViewBag.SetFocusTo = "#RepairLevels";
        }
        catch (BusinessException ex)
        {
            ViewBag.AlertMessage = ex.Message;
        }
    }

    return View("Index", repairInvoice);
}

EDIT how not to have to pass repairInvoice to business layer

// put this class in your business layer namespace
public class SerialNumberScanResult
{
    public RmaDetail RmaDetail { get; private set; }
    public bool UnderWarranty { get; private set; }

    public SerialNuberScanResult(RmaDetail rmaDetail, bool underWarranty)
    {
        RmaDetail = rmaDetail;
        UnderWarranty = underWarranty;
    }
}


public SerialNuberScanResult SerialNumberScan(int rmaNumber, int serialNumber) {
    var rmaDetail = SelectRmaDetail(rmaNumber, serialNumber);

    if (rmaDetail != null) throw new BusinessException("Serial Number not found.");

    if (rmaDetail.Received != 1) throw new BusinessException("Line item has not been received.");

    bool? workOrderClosed = WorkOrderClosed(rmaDetail.LNSEQNBR, rmaDetail.LNSEQNBR, serialNumber);

    if (!workOrderClosed.HasValue) throw new BusinessException("Work order not found.");

    if (workOrderClosed.Value == false) throw new BusinessException("Work order not closed.");

    bool underWarranty = RepairInvoiceDataLayer.UnderWarranty(rmaDetail.RETDOCID, rmaDetail.LNSEQNBR, serialNumber, rmaDetail.CUSTNMBR);

    return new SerialNuberScanResult(rmaDetail, underWarranty);
}

public ActionResult SerialNumberScan(string rmaNumber, string serialNumber)
{
    if (ModelState.IsValid)
    {
        ModelState["SerialNumber"].Value = new ValueProviderResult(serialNumber, serialNumber, CultureInfo.CurrentCulture);
        try
        {
            var result = BusinessLayer.SerialNumberScan(rmaNumber, serialNumber);
            repairInvoice.TerminalType = result.RmaDetail.ITEMNMBR.Trim();
            repairInvoice.UnderWarranty = result.UnderWarranty;
            Session["RmaDetail"] = result.RmaDetail;
            ViewBag.SetFocusTo = "#RepairLevels";
        }
        catch (BusinessException ex)
        {
            ViewBag.AlertMessage = ex.Message;
        }
    }

    return View("Index", repairInvoice);
}