Repository for Dentist and Patient Appointments

Hi,

I'm making a shared controller for dentists and patients, to show their appointments. I made a repo to keep the controller clean and added the following methods to begin.

I'm wondering, is this good practice? The right way to go about this? Looking forward to your ideas!

class AppointmentRepository
{

    private $appointment;

    public function __construct(Appointment $appointment)
    {
        $this->appointment = $appointment;
    }

    public function getAll()
    {
        return $this->appointment->all();
    }

    public function getAllByDentistId($id)
    {
        return $this->appointment->where('dentist_id', $id)->get();
    }

    public function getAllByPatientId($id)
    {
        return $this->appointment->where('patient_id', $id)->get();
    }

    public function getOneByDentistId($dentistId, $appointmentId)
    {
        return $this->appointment->where('dentist_id', $dentistId)->where('id', $appointmentId)->first();
    }

    public function getOneByPatientId($patientId, $appointmentId)
    {
        return $this->appointment->where('patient_id', $patientId)->where('id', $appointmentId)->first();
    }

    public function getConfirmedByPatientId($id)
    {
        return $this->appointment->where('patient_id', $id)->confirmed()->get();
    }

    public function getConfirmedByDentistId($id)
    {
        return $this->appointment->where('dentist_id', $id)->confirmed()->get();
    }
}

pmall

9 years ago

Best Answer

Level 56

Why don't you directly use relationships?

For example instead of getAllByDentistId, just do $dentist->appointments.

I usually advice against repository pattern. What if you need more conditions for getConfirmedByDentistId, you'll end up with getConfirmedAndSomethingByDentistId and soon it will become unmanageable. With eloquent you just : $dentist->appointments()->confirmed()->something()->get();. More flexible and elegant.

1 like

Narco

9 years ago

Level 5

Well I would suggest in making it a bit more abstract and having something like:

public function findOneBy([$field1 => $value1, $field2 => $value2, ...],  $columns = ['*'])
    {
    ...
    }

    public function findAllBy([$field1 => $value1, $field2 => $value2, ...], $columns = ['*'])
    {
    ...
    }

Because as @pmall said, when you start having a lot of functions like that it is a clear sign that you need to abstract something.

1 like

pmall

9 years ago

Level 56

@narco what you wrote above is basically wrapping eloquent in methods for the sake of wrapping it in method. What the point of using findAllBy when you can directly use $model->get().

When you have the same big query used in many places, you can wrap it into a class and thats it.

Narco

9 years ago

Level 5

@pmall It's not exactly just for the sake of wrapping it in methods.

$this->appointmentRepository->findAllBy([
    'dentist_id' => $dentistId,
    'confirmed' => true,
    'anyOtherValueYouWantToCheckFor' => $value
]);

For me it looks and reads better than $dentist->appointments()->confirmed()->something()->somethingElse()->get();

And a lot better than having tons of getConfirmedByDentistId(), getConfirmedAndSomethingByDentistId(), getOneByDentistId() methods.

pmall

9 years ago

Level 56

@narco yes but if you need multiple values you can create an appropriate scope :

$dentist->appointements()->someScope([
    'confirmed' => true,
    'anyOtherValueYouWantToCheckFor' => $value,
])->get();

You can even create customized relationships with a scope by default :

class Dentist extends Model
{
    public function confirmed_appointments()
    {
        $this->hasMany(Appointment::class)->confirmed();
    }
}

$dentist->confirmed_appointments;

Please or to participate in this conversation.