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\$ 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\$
    – EresDev
    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\$ 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\$
    – EresDev
    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\$
    – EresDev
    Dec 3, 2018 at 16:06

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.