1
\$\begingroup\$

I am starting to get into OOP using PHP and MVC. I just want to know if this code is good practice. Is this how I should be using my controller and model, in its simplest form?

AppointmentsController:

public function index()
{
    $appointmentsByLabel        = Appointment::findAppointmentsByLabel('2');
    $appointmentsBetweenDates   = Appointment::findAppointmentsBetweenDates('2016-02-01', '2016-03-31');

    return view('appointments', compact('appointmentsBetweenDates', 'appointmentsByLabel'));
}

Model (Appointment):

public static function findAppointmentsBetweenDates($start, $end)
{
    $appointments = Appointment::whereBetween('start',array($start,$end))->get();

    return $appointments;
}

public static function findAppointmentsByLabel($value)
{
    $appointments = Appointment::where('label_id', '=', $value)->get();

    return $appointments;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Could you add the name of the classes? The model class is named Appointment, right? It's a bit confusing, as you are using Appointment inside the class, instead of this. If you want to, you could also add the whereBetween and where methods, for a bit more context. But generally, this seems like an alright structure. \$\endgroup\$
    – tim
    Jan 21 '16 at 21:34
  • \$\begingroup\$ @tim, I've indeed called my model Appointment. I am not using this because the method should be static. Or I am missing something :) \$\endgroup\$
    – Hardist
    Jan 21 '16 at 21:38
  • \$\begingroup\$ @RonBrouwers oh, sorry, I overlooked the static. Yes, this doesn't work then. \$\endgroup\$
    – tim
    Jan 21 '16 at 21:43
2
\$\begingroup\$

The structure looks ok to me (I'm not that familiar with laravel, but this is basically how MVC in a web context generally looks). A couple of minor points:

  • you don't really need one-time variables such as $appointments, just return directly.
  • findAppointments* could be renamed to find*. It's in the Appointment class, so it's expected that it will find an appointment, unless noted otherwise. This also avoids the odd phrasing of Appointment::findAppointments and makes it Appointment::find.
  • be as explicit with your variable names as possible (here, it's not so important, but it's good practice, and for larger methods it does get important). eg value could be label, or even labelId, and start/end could be startDate/endDate (it may seem that this is a contradiction with the point above, but it's really not; the variables will be used inside the method, missing the direct context of the method name, while the methods will be called either directly on the class as you are doing, or on a variable, which will be named after the class, giving them direct context).
  • You can use self:: instead of ClassName::, even in static methods.
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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