1
\$\begingroup\$

I've been in an interview some time ago. They objected my Symfony API code with following line:

Too much code in controller actions. Can be moved to services to keep the business logic apart from the representation. Also violating single responsibility.

I have been trying to improve my code since then. Trying to bring SOLID in my code design and writing services for different tasks.

Now I am working on an app where I have written a controller. It still looks bulky even though it is just using services.

There are two methods:

  1. Register, to register user, add user to database and send activation code to their email
  2. Activate, get activation code from URL and activate the user or show error.

I want to know if there is any room for improvement here.

namespace App\Controller;

use App\Entity\User;
use App\Form\UserType;
use App\Serializer\FormErrorSerializer;
use App\Utils\ActivationKeyManager;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Generator\UrlGenerator;
use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;
use Symfony\Component\Translation\TranslatorInterface;

/**
 * Class AuthController
 * @package App\Controller
 */
class AuthController extends AbstractController
{
    /**
     * @param Request $request
     * @param TranslatorInterface $translator
     * @param EntityManagerInterface $em
     * @param UserPasswordEncoderInterface $encoder
     * @param FormErrorSerializer $formErrorSerializer
     * @param ActivationKeyManager $activationKeyManager
     * @param \Swift_Mailer $mailer
     * @return JsonResponse
     */
    public function register(
        Request $request,
        TranslatorInterface $translator,
        EntityManagerInterface $em,
        UserPasswordEncoderInterface $encoder,
        FormErrorSerializer $formErrorSerializer,
        ActivationKeyManager $activationKeyManager,
        \Swift_Mailer $mailer
    ) : JsonResponse {
        $form = $this->createForm(UserType::class);
        $form->handleRequest($request);

        if ($form->isSubmitted() && $form->isValid()) {
            /**
             * @var User $user
             */
            $user = $form->getData();
            $encoded = $encoder->encodePassword($user, $user->getPlainPassword());
            $user->setPassword($encoded);
            $em->persist( $user );
            $em->flush();

            $activationLink = $this->generateUrl(
                'activate',
                [
                    'code' => $activationKeyManager->getKey($user),
                    'user' => $user->getId()
                ],
                UrlGenerator::ABSOLUTE_URL
            );
            $message = (new \Swift_Message($translator->trans('Activate your account')))
                ->setFrom($this->getParameter('admin_email'))
                ->setTo($user->getEmail())
                ->setBody(
                    $this->renderView(
                        'emails/registration.txt.twig',
                        ['activationLink' => $activationLink]
                    ),
                    'text/plain'
                );

            $mailer->send($message);

            return new JsonResponse(['status' => 'success' , 'details' => $translator->trans('User has been created successfully.')]);
        }

        return new JsonResponse(['status' => 'error' , 'details' => $formErrorSerializer->convertFormToArray($form)], Response::HTTP_BAD_REQUEST);
    }

    /**
     * @param Request $request
     * @param TranslatorInterface $translator
     * @param EntityManagerInterface $em
     * @param ActivationKeyManager $activationKeyManager
     * @return JsonResponse
     */
    public function activate(
        Request $request,
        TranslatorInterface $translator,
        EntityManagerInterface $em,
        ActivationKeyManager $activationKeyManager
    ) {
        $user_id = $request->get('user');
        $activationCode = $request->get('code');

        if (!$user_id || !$activationCode) {
            return $this->sendInvalidActionCodeResponse($translator);
        }
        /**
         * @var User $user
         */
        $user = $em->getRepository(User::class)->find($user_id);
        if (!$user) {
            return $this->sendInvalidActionCodeResponse($translator);
        }

        if ($activationKeyManager->keyIsValid($user, $activationCode)) {
            $user->setActive(true);
            $em->flush($user);
            return new JsonResponse(['status' => 'success' , 'details' => $translator->trans('Your account has been activated.')]);
        } else {
            return $this->sendInvalidActionCodeResponse($translator);
        }
    }

    /**
     * @param TranslatorInterface $translator
     * @return JsonResponse
     */
    private function sendInvalidActionCodeResponse(TranslatorInterface $translator)
    {
        return new JsonResponse(['status' => 'error' , 'details' => $translator->trans('The provided activation code is not valid.')], Response::HTTP_BAD_REQUEST);
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Can you be more specific in your title and description? "Controller" is a very broad purpose for your code! \$\endgroup\$ Commented Dec 3, 2018 at 14:51
  • \$\begingroup\$ @TobySpeight I am not sure how can I be more specific. Symfony is an MVC framework and the above code is of a controller in which each public method above handles a specific request and produces response for that request with the help of services. \$\endgroup\$ Commented Dec 3, 2018 at 15:30
  • \$\begingroup\$ What distinguishes your controller from any other? I don't know the library or language, but if there was only one reasonable controller, wouldn't that just be provided? \$\endgroup\$ Commented Dec 3, 2018 at 15:38
  • \$\begingroup\$ Here is how Symfony defines a controller: "A controller is a PHP function you create that reads information from the Request object and creates and returns a Response object. The response could be an HTML page, JSON, XML, a file download, a redirect, a 404 error or anything else you can dream up. The controller executes whatever arbitrary logic your application needs to render the content of a page." My controller can be different from other controller only by being better that is, doing minimum number of tasks by itself and assigning maximum tasks to other services (classes). \$\endgroup\$ Commented Dec 3, 2018 at 16:05
  • \$\begingroup\$ There is going to be a different controller for almost every different request (user registeration, user sign in, newsletter, contact etc.) So, no there is not going to be only one controller. \$\endgroup\$ Commented Dec 3, 2018 at 16:06

0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.