1
\$\begingroup\$

I want to make the code below prettier but I don't know what to change and how to change. I know that the controller should be easy, delegating actions related to models or to services or something else.

The task the code below solves is to authenticate a client with 2 factory authentication. If a client has 2fa enabled, then additional field 2faCode is required, otherwise only email and password are required. I really don't like so many ifs for such a simple task. The code should be easy and readable.

/**
* Get a JWT via given credentials.
*
* @param LoginUserRequest $request
* @return JsonResponse
*/
public function login(LoginUserRequest $request): JsonResponse
{
   $data = $request->validated();
   if (!$token = auth()->attempt(['email' => $data['email'], 'password' => $data['password']])) {
       return response()->json(['error' => 'Unauthorized. Credentials are wrong'], 401);
   }
   $user = auth()->user();
   if (!$user->has2fa) {
       return $this->respondWithToken($token);
   }
   if (!isset($data['2faCode'])) {
       return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
   }
   $checkResult = $this->googleAuthenticator->verifyCode(env('GOOGLE_AUTHENTICATOR_SECRET'), $data['2faCode'], 10);
   if ($checkResult) {
      return $this->respondWithToken($token);
   } else {
      return response()->json(['error' => 'Unauthorized. Wrong 2faCode.'], 401);
   }}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Aug 2, 2021 at 14:57
  • \$\begingroup\$ @sko $checkResult is a single-use variable, right? I'd not declare it at all if you aren't going to use it more than once. (same with $user) \$\endgroup\$ Aug 2, 2021 at 23:40
  • \$\begingroup\$ Although it is not related to the main question, you should not use env('GOOGLE_AUTHENTICATOR_SECRET') in your code. You should use config('GOOGLE_AUTHENTICATOR_SECRET') as it will be cached and is more optimized. \$\endgroup\$ Aug 9, 2021 at 5:37

1 Answer 1

1
\$\begingroup\$

I actually think you are doing it ok. You made the choice to test a number of conditions in a logical order and return early in case of failure. Thus each test takes only a few lines of code and is clearly delimited. The control flow is easy to grasp.

You could have done it differently using nested ifs and testing for positive conditions like this:

{
   $data = $request->validated();
   if ($token = auth()->attempt(['email' => $data['email'], 'password' => $data['password']])) {
       $user = auth()->user();
       if (!$user->has2fa) {
           return $this->respondWithToken($token);
       }
       if (isset($data['2faCode'])) {
           $checkResult = $this->googleAuthenticator->verifyCode(env('GOOGLE_AUTHENTICATOR_SECRET'), $data['2faCode'], 10);
           if ($checkResult) {
              return $this->respondWithToken($token);
           } else {
              return response()->json(['error' => 'Unauthorized. Wrong 2faCode.'], 401);
           }
       } else {
           return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
       }
   } else {
       return response()->json(['error' => 'Unauthorized. Credentials are wrong'], 401);
   }

}

Pardon the poor transposition. You get the point: this approach is much less comprehensible because the whole context has to be analyzed.

Now:

if (!$user->has2fa) {
   return $this->respondWithToken($token);
}

It's possible I misunderstood but did you mean:

if ($user->has2fa) {
   return $this->respondWithToken($token);
}

If that is the case, and barring any possible misunderstanding on my end then it may be justified to combine the two ifs in a nested block. Possibly like this:

if (!$user->has2fa) {
    if (!isset($data['2faCode'])) {
       return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
    }
    return $this->respondWithToken($token);
}

One simple thing you could also have done is simply add line spacing between your blocks.

There is lots of ifs you say, in fact there are just four and if you are going to test disparate conditions there are not many better ways. A switch/case block is a possibility with some adjustments but I don't see the added value here. You would end up with a bigger control block like the counter-example above.

To sum up I think your code is ok as-is, it's about 15 lines of effective code and not really screaming for a massive refactor.

\$\endgroup\$
1
  • \$\begingroup\$ The main problem is where that code is written and what problem that code solves. That code is written inside of controller method and that code is related to business logic. Controllers should not be fat. I think about creating an object which will do authentication and 2 factory authentication. This object will provide high-level interface. \$\endgroup\$ Aug 3, 2021 at 8:58

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.