We are fully in the swing of development now, so I’ve been thinking a lot about code quality lately. This is the first time I’ve ever used PHP or Laravel, so I’m still learning about how everything works and what the best practices are. I know that there must be a lot of things that I could improve. Laravel made it easy to get started and develop a prototype quickly, but now I’m much more focused on making sure the codebase is clean, maintainable, and scalable as we continue to add features.
The prompt for this post asks us to read an article on clean code, and another on code smells. Since I’m working with Laravel, I looked for articles that were specifically about clean code and code smells in Laravel. The articles that I found to be most helpful were Understanding Code Smells in PHP and Laravel by Sohag Hasan and Writing Clean Code in Laravel by Manish Kumar.
One thing I want to start doing, which was mentioned in both articles and was something I didn’t know about, is to use service classes to encapsulate logic and keep controllers small. We have only implemented a relatively small amount of “business” logic at this point, so excessively large classes aren’t a huge problem yet, but I can see how it would be almost an inevitability without deliberate efforts to separate concerns.
I found a controller class in our codebase that does have some logic that seems like it could be moved into a service class. The controller, named FeedbackController, handles listing, storing, and searching patient feedback. It also handles creating a new patient if feedback is submitted for a patient that does not exist.
class FeedbackController extends Controller
{
public function index()
{
$feedback = PatientFeedback::orderBy('created_at', 'desc')->get();
return view('feedback.index', compact('feedback'));
}
public function store(Request $request)
{
$validated = $request->validate([
'fname' => 'required|max:255',
'lname' => 'required|max:255',
'feedback' => 'required'
]);
$validated['fname'] = htmlspecialchars($validated['fname']);
$validated['lname'] = htmlspecialchars($validated['lname']);
$patient = Patient::where('first_name', $validated['fname'])
->where('last_name', $validated['lname'])
->first();
if (!$patient) {
$patient = Patient::create([
'first_name' => $validated['fname'],
'last_name' => $validated['lname']
]);
}
PatientFeedback::create([
'patient_id' => $patient->patient_id,
'feedback' => $validated['feedback']
]);
return redirect()->route('feedback')
->with('success', 'Feedback added successfully!');
}
public function search(Request $request)
{
$name = $request->input('search_name');
$feedback = PatientFeedback::whereHas('patient', function ($query) use ($name) {
$query->where('first_name', 'like', "%{$name}%")
->orWhere('last_name', 'like', "%{$name}%");
})->orderBy('created_at', 'desc')->get();
return view('feedback.index', compact('feedback'))->with('search_name', $name);
}
}We could break out the logic for validation, storing, listing, and searching into a dedicated service class like this:
class FeedbackService
{
public function validateFeedback(array $data)
{
$validator = Validator::make($data, [
'fname' => 'required|max:255',
'lname' => 'required|max:255',
'feedback' => 'required',
]);
if ($validator->fails()) {
throw new \Illuminate\Validation\ValidationException($validator);
}
$data['fname'] = htmlspecialchars($data['fname']);
$data['lname'] = htmlspecialchars($data['lname']);
return $data;
}
public function saveFeedback(array $data)
{
$patient = Patient::firstOrCreate(
['first_name' => $data['fname'], 'last_name' => $data['lname']],
['first_name' => $data['fname'], 'last_name' => $data['lname']]
);
return PatientFeedback::create([
'patient_id' => $patient->patient_id,
'feedback' => $data['feedback']
]);
}
public function getAllFeedback()
{
return PatientFeedback::orderBy('created_at', 'desc')->get();
}
public function searchFeedbackByName(string $name)
{
return PatientFeedback::whereHas('patient', function ($query) use ($name) {
$query->where('first_name', 'like', "%{$name}%")
->orWhere('last_name', 'like', "%{$name}%");
})->orderBy('created_at', 'desc')->get();
}
}And then the FeedbackController would be quite a bit simpler:
class FeedbackController extends Controller
{
protected $feedbackService;
public function __construct(FeedbackService $feedbackService)
{
$this->feedbackService = $feedbackService;
}
public function index()
{
$feedback = $this->feedbackService->getAllFeedback();
return view('feedback.index', compact('feedback'));
}
public function store(Request $request)
{
try {
$this->feedbackService->saveFeedback($request->all());
return redirect()->route('feedback')
->with('success', 'Feedback added successfully!');
} catch (\Illuminate\Validation\ValidationException $e) {
return redirect()->back()->withErrors($e->validator)->withInput();
}
}
public function search(Request $request)
{
$name = $request->input('search_name');
$feedback = $this->feedbackService->searchFeedbackByName($name);
return view('feedback.index', compact('feedback'))->with('search_name', $name);
}
}This way, the controller focuses on handling HTTP requests, while the service class encapsulates the logic for managing feedback. It makes the code cleaner and also makes it easier to test each layer independently.
I would like for us to stop putting so much logic in our controllers. It hasn’t become a huge problem yet, but it would be good to get into the habit of keeping as much logic out of the controllers as possible. Another way, beside service classes, to accomplish this (which I also just learned about) it to use Laravel’s form request classes for validation logic.
For example, in our FeedbackController, the store method currently has inline validation and sanitization:
public function store(Request $request)
{
$validated = $request->validate([
'fname' => 'required|max:255',
'lname' => 'required|max:255',
'feedback' => 'required'
]);
$validated['fname'] = htmlspecialchars($validated['fname']);
$validated['lname'] = htmlspecialchars($validated['lname']);
// ...
}If we make a new request class to encapsulate this logic, Laravel will automatically validate the request when the class is type-hinted in the controller.
class FeedbackRequest extends FormRequest
{
public function rules()
{
return [
'fname' => 'required|max:255',
'lname' => 'required|max:255',
'feedback' => 'required',
];
}
protected function prepareForValidation()
{
$this->merge([
'fname' => htmlspecialchars($this->input('fname')),
'lname' => htmlspecialchars($this->input('lname')),
]);
}
}With that in place, the FeedbackController‘s store method is much simpler:
public function store(FeedbackRequest $request)
{
$validated = $request->validated(); // Automatically validated and sanitized
$this->feedbackService->saveFeedback($validated);
return redirect()->route('feedback')
->with('success', 'Feedback added successfully!');
}Writing code like this is beneficial because it promotes separation of concerns, reusability, and maintainability.