Laravel controller doing too many things

[rathorevaibhav]

Questions

In the following PHP/Laravel code:

1. Where do you see a violation of the Single Responsibility Principle?
2. What other SOLID principles do you see not being followed?
3. Can you share a better, more efficient, more developer-friendly version of the code below?

public function store(Request $request)
{
    $data = [
        'activity_name' => $request->input('activity_name'),
        'activity_start_date' => $request->input('activity_start_date'),
        'activity_city' => $request->input('activity_city'),
        'timeslot' => $request->input('timeslot'),
        'pickup_preference' => $request->input('pickup_preference'),
        'user_name' => $request->input('user_name')
    ];
    $userEmail = $request->user_email;

    // Check if user has already applied for an activity on the same day
    $activity = Activities::find($request->input('activity_id'));

    $existingActivityUser = ActivityUser::where('user_id', $request->input('user_id'))
        ->whereHas('activity', function ($query) use ($activity) {
            $query->where('start_date', $activity->start_date);
        })
        ->where('has_withdrawn', 0)
        ->first();

    if ($existingActivityUser) {
        return redirect()->back()->withErrors([
            'error' => 'We are sorry, but you have already applied for an activity that is scheduled for the same day as this one. Please choose another activity to apply for.'
        ]);
    }

    $activityUser = ActivityUser::where([
        ['user_id', '=', $request->input('user_id')],
        ['activity_id', '=', $request->input('activity_id')],
    ])->first();

    if ($activityUser) {
        // Update the existing record
        $activityUser->my_timeslote_preference = $request->input('timeslot');
        $activityUser->my_pickup_preference = $request->input('pickup_preference');
        $activityUser->has_withdrawn = false;
        $activityUser->reason_for_withdrawal = null;
        $activityUser->save();

        Mail::to($userEmail)->queue(new ActivityInfo($data));
    } else {
        // Create a new record
        $activityUser = new ActivityUser();
        $activityUser->activity_id = $request->input('activity_id');
        $activityUser->user_id = $request->input('user_id');
        $activityUser->my_timeslote_preference = $request->input('timeslot');
        $activityUser->my_pickup_preference = $request->input('pickup_preference');
        $activityUser->save();

        Mail::to($userEmail)->queue(new ActivityInfo($data));
    }

    return redirect()->route('activities.index')->with([
        'success' => 'Thank you for your interest in applying for the volunteering activity. You can now view the details of the activity in the `My Activities` section.'
    ]);
}

[Sachinbisht27]

I think this function has multiple responsibilities, such as handling updating or creating activity records, checking for existing activities, and sending emails.
And what I found in the research of SRP (Single Responsibility Principle) is that a function or module should have only one reason to change, which means that it should have a single responsibility only.
That's why this code is violating the Single Responsibility Principle.

[Satendra-SR]

Thanks, @Sachinbisht27 for making an attempt 🙌. Concept-wise, you pointed it right. Would you like to try fixing it by sharing your version? Let's say you have to write the same functionality, keeping SRP in mind, what it would have been?

[Sachinbisht27]

Thanks, @Sachinbisht27 for making an attempt 🙌. Concept-wise, you pointed it right. Would you like to try fixing it by sharing your version? Let's say you have to write the same functionality, keeping SRP in mind, what it would have been?

Right now, the code appears to be so much dependable but yes sure @Satendra-SR I will try for it.

[Sachinbisht27]

@Satendra-SR
We can update the code like this by making a separate function for every operation.

1. Made the store function the main function.
2. Created different functions for every operation and called them in the main function:
   • getActivityData for fetching data from the request.
   • hasAlreadyAppliedForActivity for checking for existing activities.
   • updateActivityUser for updating the existing data for users in ActivityUser.
   • createActivityUser for creating new entries in the ActivityUser.
   • sendActivityInfoEmail for sending mail to users.

public function store(Request $request)
{
    $data = $this->getActivityData($request);

    $userEmail = $request->user_email;

    if ($this->hasAlreadyAppliedForActivity($request)) {
        return redirect()->back()->withErrors([
            'error' => 'We are sorry, but you have already applied for an activity that is scheduled for the same day as this one. Please choose another activity to apply for.'
        ]);
    }

    $activityUser = ActivityUser::where([
        ['user_id', '=', $request->input('user_id')],
        ['activity_id', '=', $request->input('activity_id')],
    ])->first();

    if ($activityUser) {
        $this->updateActivityUser($activityUser, $request);
    } else {
        $activityUser = $this->createActivityUser($request);
    }

    $this->sendActivityInfoEmail($userEmail, $data);

    return redirect()->route('activities.index')->with([
        'success' => 'Thank you for your interest in applying for the volunteering activity. You can now view the details of the activity in the `My Activities` section.'
    ]);
}

public function getActivityData(Request $request)
{
    return [
        'activity_name' => $request->input('activity_name'),
        'activity_start_date' => $request->input('activity_start_date'),
        'activity_city' => $request->input('activity_city'),
        'timeslot' => $request->input('timeslot'),
        'pickup_preference' => $request->input('pickup_preference'),
        'user_name' => $request->input('user_name')
    ];
}

public function hasAlreadyAppliedForActivity(Request $request)
{
    $activity = Activities::find($request->input('activity_id'));

    $existingActivityUser = ActivityUser::where('user_id', $request->input('user_id'))
        ->whereHas('activity', function ($query) use ($activity) {
            $query->where('start_date', $activity->start_date);
        })
        ->where('has_withdrawn', 0)
        ->first();

    return $existingActivityUser;
}

public function createActivityUser(Request $request)
{
    $activityUser = new ActivityUser();
    $activityUser->activity_id = $request->input('activity_id');
    $activityUser->user_id = $request->input('user_id');
    $activityUser->my_timeslote_preference = $request->input('timeslot');
    $activityUser->my_pickup_preference = $request->input('pickup_preference');
    $activityUser->save();

    return $activityUser;
}

public function updateActivityUser(ActivityUser $activityUser, Request $request)
{
    $activityUser->my_timeslote_preference = $request->input('timeslot');
    $activityUser->my_pickup_preference = $request->input('pickup_preference');
    $activityUser->has_withdrawn = false;
    $activityUser->reason_for_withdrawal = null;
    $activityUser->save();
}


public function sendActivityInfoEmail($userEmail, $data)
{
    Mail::to($userEmail)->queue(new ActivityInfo($data));
}

[Abhishek-Khanduri]

As in the above-provided code, the store function is performing the following tasks:

1. Check if user has already applied for an activity on the same day
2. Update the existing record
3. Create a new record
4. Sends mail to the user.
5. Redirects the user to the activities.index page.
   so this method is responsible for doing these many tasks and is performing more than one functionality and hence violating the Single Responsibility Principle.

One way that I can think of to resolve this is by creating separate functions to handle different responsibilities and then using them here.

[pokhiii]

One way that I can think of to resolve this is by creating separate functions to handle different responsibilities and then using them here

@Abhishek-Khanduri Can you show us how with updated code example?

[rohit123-rawat]

1. Make hasAlreadyAppliedForActivity function to check if user has already applied for an activity on the same day and call it in store function and throw errors if already applied.
2. Reduce 'activity_name' => $request->input('activity_name') by replacing $data = $request->only([activity_name]).
3. Use UpdateOrCreate function instead of checks.

In this Not sure about create or update function that it work properly. For that need to test first.

  public function store(Request $request)
   {
       $data = $request->only([
           'activity_name',
           'activity_start_date',
           'activity_city',
           'timeslot',
           'pickup_preference',
           'user_name'
       ]);

       $userEmail = $request->user_email;

       if ($this->hasAlreadyAppliedForActivity($request)) {
           return redirect()->back()->withErrors([
               'error' => 'We are sorry, but you have already applied for an activity that is scheduled for the same day as this one. Please choose another activity to apply for.'
           ]);
       }

       // create and update record

       $activityUser = ActivityUser::updateOrCreate(
           [
               'user_id' => $request->input('user_id'),
               'activity_id' => $request->input('activity_id')
           ],
           [
               'my_pickup_preference' => $request->input('pickup_preference'),
               'my_timeslot_preference' => $request->input('timeslot'),
               'has_withdrawn' => false,
               'reason_for_withdrawal' => null
           ]
       );
       $activityUser->save();

       Mail::to($userEmail)->queue(new ActivityInfo($data));

       return redirect()->route('activities.index')->with([
           'success' => 'Thank you for your interest in applying for the volunteering activity. You can now view the details of the activity in the `My Activities` section.'
       ]);
   }

   public function hasAlreadyAppliedForActivity(Request $request)
   {
       // Check if user has already applied for an activity on the same day
       
       $activity = Activities::find($request->input('activity_id'));

       $existingActivityUser = ActivityUser::where('user_id', $request->input('user_id'))
           ->whereHas('activity', function ($query) use ($activity) {
               $query->where('start_date', $activity->start_date);
           })
           ->where('has_withdrawn', 0)
           ->first();

       return $existingActivityUser;
   }
   

[rathorevaibhav]

I think it's a good thought. hasAlreadyAppliedForActivity can be a separate function which can be reused in other features/flows also.

I see there is still scope to remove code from the store and move it to separate functions. Maybe have separate functions/components for:

1. Request data validation
2. Mail trigger too maybe

[rohit123-rawat]

1. Get request inputs from validation as $data variable.
2. make hasAlreadyAppliedForActivity function to check user applied or not.
3. make create and update logic in store function.
4. make activityInfoMail function to trigger the mail.

   public function store(ActivityUserRequest $request)
   {
      // get data from the request file 
      
       $data = $request->validate();

       $userEmail = $request->user_email;

        // check the user already applied.
        
       if ($this->hasAlreadyAppliedForActivity($request)) {
           return redirect()->back()->withErrors([
               'error' => 'We are sorry, but you have already applied for an activity that is scheduled for the same day as this one. Please choose another activity to apply for.'
           ]);
       }

       // create or update the record

       $activityUser = ActivityUser::updateOrCreate(
           [
               'user_id' => $request->input('user_id'),
               'activity_id' => $request->input('activity_id')
           ],
           [
               'my_pickup_preference' => $request->input('pickup_preference'),
               'my_timeslot_preference' => $request->input('timeslot'),
               'has_withdrawn' => false,
               'reason_for_withdrawal' => null
           ]
       );
       $activityUser->save();
       
    // Send Activity information mail to the user 
    
       $this->activityInfoMail($userEmail, $data);

       return redirect()->route('activities.index')->with([
           'success' => 'Thank you for your interest in applying for the volunteering activity. You can now view the details of the activity in the `My Activities` section.'
       ]);
   }

   public function activityInfoMail($userEmail, $data)
   {
       // function to send an email with the activity information to the user.
       Return Mail::to($userEmail)->queue(new ActivityInfo($data));
   }

   public function hasAlreadyAppliedForActivity(Request $request)
   {
       // Check if user has already applied for an activity on the same day

       $activity = Activities::find($request->input('activity_id'));

       $existingActivityUser = ActivityUser::where('user_id', $request->input('user_id'))
           ->whereHas('activity', function ($query) use ($activity) {
               $query->where('start_date', $activity->start_date);
           })
           ->where('has_withdrawn', 0)
           ->first();

       return $existingActivityUser;
   }

[belwalshubham]

From my understanding the SRP (single responsibility function ) tell that a function or class has only one functionality not multiple.
so it's better if we have one responsibility at a time because then the code will be more clear and optimize
but here the store function is doing multiple things such as handling the activity, extracting the email and updating records, creating new records, and sending emails. so we have multiple responsibilities on this code,

so we can now optimize the code to something like that :

// checkUserExistingActivity  method checks if the user has already applied for an activity for same day.

private function checkUserExistingActivity(Request $request)
{
// checking existing activity user code is something like that
  $activity = Activities::find($request->input('activity_id'));

  $existingActivityUser = ActivityUser::where('user_id', $request->input('user_id'))
      ->whereHas('activity', function ($query) use ($activity) {
          $query->where('start_date', $activity->start_date);
      })
      ->where('has_withdrawn', 0)
      ->first();
}

// and then create different function for update user activty.


private function updateUserActivity($request) 
{
  // update user activity code 
}

// and then same for sending email 

private function sendEmailActivity ($userEmail, $data)
{
// function to send an email with the activity information to the user.
}

// so on

with this approach, we have different functions for different functionality, it avoids a Single function having multiple functionality.

[pokhiii]

Reading the code, my understanding is as follows:

1. there are 2 entities: user and activity.
2. a user can apply for an activity and this relationship is captured by ActivityUser (I'd have named it Application or ActivityApplication)
3. the store method is basically for creating (storing) a new Application
4. there is ONLY one validation, the user should not have applied for an activity before that starts on the same day

I'd split the function in the following parts and then use approaches from Laravel to simply this. The function will look like this:

// 1. data validation (required inputs and pouplating `$data` variable)

// 2. business logic validation (no conflicting application)

// 3. create new application or update existing

// 4. send the email

I will try to add actual Laravel code as I find some more time. Meanwhile if someone wants to help feel free comment :)

[mohitgusain]

Everyone has covered pretty much in their comments. I can suggest some code optimization.

1. Here, rather than manually assigning each attribute individually,

$activityUser->my_timeslote_preference = $request->input('timeslot');
$activityUser->my_pickup_preference = $request->input('pickup_preference');
$activityUser->has_withdrawn = false;
$activityUser->reason_for_withdrawal = null;
$activityUser->save();

we can use the direct create method like this way
$activityUser = ActivityUser::create($data);

I will check more and share