patterncsharpMinor
MVC refactoring parameter check
Viewed 0 times
refactoringparametercheckmvc
Problem
I had this code repeated in many Actions:
So I extracted a method to:
Which I call like so:
I don't like that I have to call CheckPatient twice if it fails and the returning of null - is there a better way to do this or is it ok?
public ActionResult History(int? patientId)
{
if (patientId == null)
{
return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
}
var patient = patientService.GetPatient(patientId.Value);
if (patient == null)
{
return HttpNotFound();
}
...
}So I extracted a method to:
private ActionResult CheckPatientId(int? patientId, ref Patient patient)
{
if (patientId == null)
{
return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
}
patient = patientService.GetPatient(patientId.Value);
if (patient == null || patient.PharmacyId != User.PharmacyId())
{
return HttpNotFound();
}
return null;
}Which I call like so:
var patient = new Patient();
if (CheckPatientId(patientId, ref patient) != null)
{
return CheckPatientId(patientId, ref patient);
}I don't like that I have to call CheckPatient twice if it fails and the returning of null - is there a better way to do this or is it ok?
Solution
The primary intent of your function is to get Patient instance. Secondary intent is to notify of the error. You should always attempt to translate the intent to code and function names. So, your function should return a
and the call would look like:
Patient instance (happy path), or an ActionResult as an out parameter in case something goes wrong. Usually an out keyword is used when the variable is initialized inside of the function, not the ref keyword. This is done mainly to make your intent more declarative and obvious.private Patient GetPatient(int? patientId, out ActionResult errorResult)
{
errorResult = null;
var patient = null;
if (patientId == null)
{
errorResult = HttpStatusCodeResult(HttpStatusCode.BadRequest);
}
else
{
patient = patientService.GetPatient(patientId.Value);
if (patient == null || patient.PharmacyId != User.PharmacyId())
{
errorResult = HttpNotFound();
}
}
return patient;
}and the call would look like:
ActionResult errorResult;
var patient = GetPatient(patientId, out errorResult);
if (errorResult!= null)
{
return errorResult;
}
//do the rest of the Patient processingCode Snippets
private Patient GetPatient(int? patientId, out ActionResult errorResult)
{
errorResult = null;
var patient = null;
if (patientId == null)
{
errorResult = HttpStatusCodeResult(HttpStatusCode.BadRequest);
}
else
{
patient = patientService.GetPatient(patientId.Value);
if (patient == null || patient.PharmacyId != User.PharmacyId())
{
errorResult = HttpNotFound();
}
}
return patient;
}ActionResult errorResult;
var patient = GetPatient(patientId, out errorResult);
if (errorResult!= null)
{
return errorResult;
}
//do the rest of the Patient processingContext
StackExchange Code Review Q#92211, answer score: 5
Revisions (0)
No revisions yet.