current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I had this code repeated in many Actions: 

    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?

share|improve this question
1  
What's the point of calling 'CheckPatientId' twice? How would the result be different the second time? You're passing in the same values. I also don't see why would you use ref. –  Daniel Sokolov May 30 at 10:45
    
I have to use ref because I cant return the patient object –  woggles May 30 at 13:28

2 Answers 2

up vote 4 down vote accepted

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 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 processing
share|improve this answer
up vote 4 down vote

No, it's not OK to call the same method twice with the same parameters. At the minimum, you can put the result of the first call in a variable:

var actionResult = CheckPatientId(patientId, ref patient);
if (actionResult != null)
{
    return actionResult;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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