laravel Cleaning code + make it work correctly

Question

hello there i have this simple update method but it has a problem the thing is the user has four conditions it is for user editing

1- old email == new email

2- old email !== new email

3- password == null

4- password !== null

so the core function is like this

            $v = Validator::make($request->all(),[
                'name_ar' => ['required', 'string', 'max:255'],
                'name_en' => ['required', 'string', 'max:255'],
                'email' => [ 'required' ,'string', 'email', 'max:255'],
                'permission' => ['required', 'string'],
                'branch' => ['required', 'integer'],    
            ]);
            if($v->fails()){
                return redirect()->back()->withErrors($v)->withInput();
            }
          $user->name_ar = $request->input('name_ar');
          $user->name_en = $request->input('name_en');
          $user->email = $request->input('email');
          $user->password = $user->password;
          if($selected_permission == 'admin'){
            $this->saveAsAdmin($request , $user);
            return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
          }
          $this->saveAsUser($request , $user);
          return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
        }

save as admin and save as user are simple methods

save as admin

public function saveAsAdmin($request , $user)
{
    $user->is_admin = 1;
    $user->is_user = 0;
    $user->branch_id = $request->input('branch');
    $user->save();
}

save as user

public function saveAsUser($request , $user)
{
    $user->is_admin = 0;
    $user->is_user = 1;
    $user->branch_id = $request->input('branch');
    $user->save();
    }

the thing is if i make 4 methods and used them in each condition it doesn't work correctly name ar + name en + permission aren't being saved to the database + validator doesn't show errors i have been struggling to make it clean for the last few hours and nothing really worked correctly so i ended up making it like this

public function update(Request $request , $id)
{
    $user = User::find($id);
    $old_email = $user->email;
    $new_email = $request->input('email');
    $selected_permission = $request->input('permission');
    if($old_email == $new_email){
        if($request->input('password') == null){
        $v = Validator::make($request->all(),[
            'name_ar' => ['required', 'string', 'max:255'],
            'name_en' => ['required', 'string', 'max:255'],
            'email' => [ 'required' ,'string', 'email', 'max:255'],
            'permission' => ['required', 'string'],
            'branch' => ['required', 'integer'],    
        ]);
        if($v->fails()){
            return redirect()->back()->withErrors($v)->withInput();
        }
      $user->name_ar = $request->input('name_ar');
      $user->name_en = $request->input('name_en');
      $user->email = $request->input('email');
      $user->password = $user->password;
      if($selected_permission == 'admin'){
        $this->saveAsAdmin($request , $user);
        return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
      }
      $this->saveAsUser($request , $user);
      return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
    }

    $v = Validator::make($request->all(),[
        'name_ar' => ['required', 'string', 'max:255'],
        'name_en' => ['required', 'string', 'max:255'],
        'email' => [ 'required' ,'string', 'email', 'max:255'],
        'password' => ['required', 'string', 'min:8',],
        'permission' => ['required', 'string'],
        'branch' => ['required', 'integer'],    
    ]);
    if($v->fails()){
        return redirect()->back()->withErrors($v)->withInput();
    }
  $user->name_ar = $request->input('name_ar');
  $user->name_en = $request->input('name_en');
  $user->email = $request->input('email');
  $user->password = bcrypt($request->input('password'));
  if($selected_permission == 'admin'){
    $this->saveAsAdmin($request , $user);
    return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
  }
  $this->saveAsUser($request , $user);
  return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');
}else{
    if($request->input('password') == null){
        $v = Validator::make($request->all(),[
            'name_ar' => ['required', 'string', 'max:255'],
            'name_en' => ['required', 'string', 'max:255'],
            'email' => ['required', 'string', 'email', 'max:255', 'unique:users'],
            'permission' => ['required', 'string'],
            'branch' => ['required', 'integer'],    
        ]);
        if($v->fails()){
            return redirect()->back()->withErrors($v)->withInput();
        }
      $user->name_ar = $request->input('name_ar');
      $user->name_en = $request->input('name_en');
      $user->email = $request->input('email');
      $user->password = $user->password;
      if($selected_permission == 'admin'){
        $this->saveAsAdmin($request , $user);
  return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');

      }
      $this->saveAsUser($request , $user);
  return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');

    }
    $v = Validator::make($request->all(),[
        'name_ar' => ['required', 'string', 'max:255'],
        'name_en' => ['required', 'string', 'max:255'],
        'email' => ['required', 'string', 'email', 'max:255', 'unique:users'],
        'password' => ['required', 'string', 'min:8'],
        'permission' => ['required', 'string'],
        'branch' => ['required', 'integer'], 
    ]);
    if($v->fails()){
        return redirect()->back()->withErrors($v)->withInput();
    }
  $user->name_ar = $request->input('name_ar');
  $user->name_en = $request->input('name_en');
  $user->email = $request->input('email');
  $user->password = $user->password;
  if($selected_permission == 'admin'){
    $this->saveAsAdmin($request , $user);
  return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');

  }
  $this->saveAsUser($request , $user);
  return redirect('super-admin/permissions/index')->with('success' , 'User Edited Successfully');

}

}

Yes It Is UGLYYY i totally agree with you but at least it works correctly , i am not new to laravel but i don't have much knowledge of making clean code in php maybe because i didn't learn pure php i just learned laravel directly

please share with me a working + clean solution so that everyone in the future + learners could improve their skills and knowledge

yes yes i said it from the memory you got the point , though thanks for the reminder i will edit it — RYOK
This is pretty pointless: $user->password = $user->password; — M. Eriksson
great i will remove it you got a point ,i will remove it immediately , but what really will make it better if we separated each block of the if statement with a will named function but when i do that the validator shows error on the index page and some information aren't being saved correctly i don't know why , any ideas ? — RYOK
There is a lot of unnecessary code for example if the user is not an admin just save it as is_admin = 0. Don't need for is_user = 1, is_admin = 0; Second advice is to mass assigned those variables, this will make the code shorter. — Syazany
its just for more protection , for example if someone knew the structure of the database and he assigned both of them to 1 using for example burp suite this will create problems in the future , and about mass assigned i will use it thanks for the advice ^_^ — RYOK

huliver huliver · Accepted Answer · 2020-07-29T02:43:15

This is simple code to update User. Maybe it can help you

public function updateUser(Request $request)
{
    $validator = Validator::make($request->all(), [
        'id' => 'required|numeric',
        'email' => 'required|max:255',
        'name' => 'required|max:255',
    ]);
    if ($validator->fails()) {
        return ['code' => "ERROR_PARAMS"];
    }
    $requestData = $request->all();

    $user = User::find($requestData['id']);
    $user->email = $requestData['email'];
    $user->name = $requestData['name'];
    // Change password if exist param 'password'
    if ($requestData['password'] != null) {
        $user->password = bcrypt($requestData['password']);
    }
    
    $user->save();
    return ['code' => "SUCCESS"];
}

laravel Cleaning code + make it work correctly

1 Answers