7
\$\begingroup\$

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?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ It's easy to write spaghetti code in any language, using any framework :) \$\endgroup\$ Nov 25, 2013 at 17:17
  • \$\begingroup\$ Hi @broke, welcome to CR! Stick around, spend some of those votes - you may or may not know, but we're on a mission here, to shoot all zombies. Join the party! \$\endgroup\$ Nov 26, 2013 at 0:40
  • \$\begingroup\$ @broke Controller = Handling of HTTP requests and the building of view-models. Data Layer = saving and retrieving of data using queries. Service Layer = business logic manipulating data objects and sometimes returning objects for the controller to use in building view-models. \$\endgroup\$ Nov 26, 2013 at 10:40
  • \$\begingroup\$ @broke You can forget about interfaces and DI for now, it's not necessary for a service layer. Just think of your service classes as a place to put business logic without the unnecessary burden of dealing with HTTP requests. \$\endgroup\$ Nov 26, 2013 at 16:57
  • \$\begingroup\$ @SilverlightFox There isn't much business logic in my controller besides setting viewbag values and trimming strings. Is that the code that should be put in a service layer? \$\endgroup\$
    – broke
    Nov 26, 2013 at 17:09

2 Answers 2

7
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ These links you provided show some very pretty pictures, but those don't help very much. If a service layer is what I need, then a how to guide with code examples would be way more helpful. I have no idea what the code for this service layer is supposed to look like, or where to put it. \$\endgroup\$
    – broke
    Nov 25, 2013 at 17:41
  • \$\begingroup\$ @broke OK, code example added. \$\endgroup\$ Nov 25, 2013 at 18:04
2
\$\begingroup\$

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);
}
\$\endgroup\$
3
  • \$\begingroup\$ The repairinvoice object that is being passed in is my model. Would I have to pass that in by ref so the changes would be reflected to my view? \$\endgroup\$
    – broke
    Dec 4, 2013 at 17:47
  • \$\begingroup\$ No you wouldn't have to pass by ref. We can further refactor to remove the need to pass repairInvoice altogether. \$\endgroup\$ Dec 4, 2013 at 19:07
  • \$\begingroup\$ @broke I edited my answer to show how not to have to pass repairInvoice to business layer. \$\endgroup\$ Dec 4, 2013 at 19:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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