patterncsharpMinor
Need help with cleaning up this Controller
Viewed 0 times
thisneedwithhelpcontrollercleaning
Problem
I've written the following controller and i was looking for some input/advice on how i could go about cleaning this up or rewriting it. I feel like i'm repeating myself a lot or that there might be a better way to do this.
```
namespace WebUI.Controllers
{
[Authorize(Roles="Administrator,Registrar")]
public class RegistrarController : BaseController
{
private readonly IRegistrationService _registrationService;
public const string WizardKey = "Wizard";
public const string ActionKey = "Action";
public RegistrarController(IRegistrationService registrationService)
{
this._registrationService = registrationService;
}
[HttpGet]
public ActionResult Step1()
{
if (string.IsNullOrEmpty(this.RegistrarId))
{
ViewBag.ErrorMessage = "A valid registrar id is not associated with this account.";
return View("Error");
}
PersonRegistrationWizard wizard;
if (!PrepareAndCheckStep(1, out wizard)) { / Should always be true... / }
return View(wizard.Step1Model);
}
[HttpPost]
public ActionResult Step1(NewRegistrantModel model)
{
var wizard = TempData[WizardKey] as PersonRegistrationWizard;
if (wizard == null)
wizard = new PersonRegistrationWizard();
if (!ModelState.IsValid)
{
wizard.Step1Model = model;
TempData[WizardKey] = wizard;
return View("Step1", model);
}
var eventDate = _registrationService.GetUpcomingEventFor(DateTime.Now);
if (!eventDate.HasValue)
{
wizard.Step1Model = model;
TempData[WizardKey] = wizard;
ViewBag.ErrorMessage = "Sorry, no upcoming event was found within our database.";
return View("Error");
}
```
namespace WebUI.Controllers
{
[Authorize(Roles="Administrator,Registrar")]
public class RegistrarController : BaseController
{
private readonly IRegistrationService _registrationService;
public const string WizardKey = "Wizard";
public const string ActionKey = "Action";
public RegistrarController(IRegistrationService registrationService)
{
this._registrationService = registrationService;
}
[HttpGet]
public ActionResult Step1()
{
if (string.IsNullOrEmpty(this.RegistrarId))
{
ViewBag.ErrorMessage = "A valid registrar id is not associated with this account.";
return View("Error");
}
PersonRegistrationWizard wizard;
if (!PrepareAndCheckStep(1, out wizard)) { / Should always be true... / }
return View(wizard.Step1Model);
}
[HttpPost]
public ActionResult Step1(NewRegistrantModel model)
{
var wizard = TempData[WizardKey] as PersonRegistrationWizard;
if (wizard == null)
wizard = new PersonRegistrationWizard();
if (!ModelState.IsValid)
{
wizard.Step1Model = model;
TempData[WizardKey] = wizard;
return View("Step1", model);
}
var eventDate = _registrationService.GetUpcomingEventFor(DateTime.Now);
if (!eventDate.HasValue)
{
wizard.Step1Model = model;
TempData[WizardKey] = wizard;
ViewBag.ErrorMessage = "Sorry, no upcoming event was found within our database.";
return View("Error");
}
Solution
Here the UI model is tightly coupled with the data structures required. Hence the need arises for transfering data from step 1 to step 2 etc.
Instead the data structures should be based on business objects required - e.g. Person object with nested objects like social security info, personal info, residence info etc. Let all steps deal with the same Person object, updating inputs given by user in each step.
Also the 'Step' itself can be separated out into an interface / abstract class. Let's say IStep is in interface with ValidateInput(), Prepare() and ExecuteStep() methods. ExecuteStep() internally calls ValidateInput() and Prepare() and returns ActionResult object.
Then we have 4 small subclasses, one for each step implementing IStep. The controller code will be simplified, it just needs to create an instance of next step (depending on ActionResult received from earlier step) and call Execute() on the same. All step objects will share the same Person object, which can be passed to the constructor.
Please reply back with specific queries if this approach is not clear.
Following are the advantages of this approach
Instead the data structures should be based on business objects required - e.g. Person object with nested objects like social security info, personal info, residence info etc. Let all steps deal with the same Person object, updating inputs given by user in each step.
Also the 'Step' itself can be separated out into an interface / abstract class. Let's say IStep is in interface with ValidateInput(), Prepare() and ExecuteStep() methods. ExecuteStep() internally calls ValidateInput() and Prepare() and returns ActionResult object.
Then we have 4 small subclasses, one for each step implementing IStep. The controller code will be simplified, it just needs to create an instance of next step (depending on ActionResult received from earlier step) and call Execute() on the same. All step objects will share the same Person object, which can be passed to the constructor.
Please reply back with specific queries if this approach is not clear.
Following are the advantages of this approach
- The controller code is much simpler
- It is easier to modify a step
- It is easier to add/remove a step. Controller class will have very minimal effect of calling the new step.Execute() or removing call for an existing step. Other steps will not be affected at all.
Context
StackExchange Code Review Q#5006, answer score: 3
Revisions (0)
No revisions yet.