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:
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.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.User clicks the process button and then the
Index()
with theHTTPPOST
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?