Laravel 5.1代码优化

Is there a way that the below code can be shortened? It's starting to look a bit messy and I wanted to know if there was a better way.

/**
 * Update user.
 *
 * @param $request
 * @param $id
 * @return mixed
 */
public function updateUser($request, $id)
{
    // Get user
    $user = $this->user->find($id);

    // Sync job titles
    if($request->has('job_title'))
    {
        $user->jobTitles()->sync((array)$request->get('job_title'));
    } else {
        $user->jobTitles()->detach();
    }

    // Sync employee types
    if($request->has('employee_type'))
    {
        $user->employeeTypes()->sync((array)$request->get('employee_type'));
    } else {
        $user->employeeTypes()->detach();
    }

    if($request->has('status')) {
        $data = $request->only('first_name', 'last_name', 'email', 'status');
    } else {
        $data = $request->only('first_name', 'last_name', 'email');
    }

    // Save user changes
    return $this->user->whereId($id)->update($data);
}

Any help would be appreciated.

Here is what I would have done:

• Extracted the ManyToMany relationships to their own methods.
• Initialized the $data variable and overridden it if necessary.
• Removed the comments. The code is readable enough, no need for them

Code:

public function updateUser($request, $id)
{
    $user = $this->user->find($id);
    $data = $request->only('first_name', 'last_name', 'email');

    $this->syncJobTitles($user);
    $this->syncEmployeeTypes($user);

    if($request->has('status')) {
        $data['status'] = $request->status;
    }

    return $user->update($data);
}

private function syncJobTitles($user)
{
    if(request()->has('job_title'))
    {
        $user->jobTitles()->sync((array) request()->get('job_title'));
    } else {
        $user->jobTitles()->detach();
    }
}

private function syncEmployeeTypes($user)
{
    if(request()->has('employee_type'))
    {
        $user->employeeTypes()->sync((array) request()->get('employee_type'));
    } else {
        $user->employeeTypes()->detach();
    }
}

There are different ways to refactor that code:

If that code is only used on that part of your code you could leave it there or

Option 1 Move that business logic to the user model

class User extends Eloquent 
{
  ...
  public function applySomeRule($request) 
  {
    if($request->has('job_title')) {
      $this->jobTitles()->sync((array)$request->get('job_title'));
    } else {
      $this->jobTitles()->detach();
    }

    // Sync employee types
    if($request->has('employee_type')) {
      $this->employeeTypes()->sync((array)$request->get('employee_type'));
    } else {
      $this->employeeTypes()->detach();
    }
  }
}

And your controller could finish like

public function updateUser($request, $id)
{
    // Get user
    $user = $this->user->find($id);
    $user->applySomeRule($request);

    if($request->has('status')) {
        $data = $request->only('first_name', 'last_name', 'email', 'status');
    } else {
        $data = $request->only('first_name', 'last_name', 'email');
    }

    // Save user changes
    return $this->user->whereId($id)->update($data);
}

Option 2 If that business logic is used on different controller methods you can use Middlewares, so that you move that logic to middleware and in your route definition you use the middleware you created, let's say SomeRuleMiddleware.

Your routes would look like:

Route::put('user/{id}', [
     'middleware' => 'SomeRuleMiddleware', 
     'uses' => 'YourController@updateUser'
]);

Option 3 You could move all your business logic to Repositories (read about Repository Pattern) and SOLID principles, that way your logic and rules will keep on Repositories and your controllers would keep clean, something like this:

class YourController extends Controller 
{
  protected $userRepo;

  public function __construct(UserRepository $userRepo)
  {
    $this->userRepo = $userRepo;
  }

  public function updateUser($request, $id)
  {
    $data = $request->all();
    $result = $this->userRepo->updateUser($id, $data);

    return $result;
  }
}