-5
\$\begingroup\$

Here's a sample code that I made up :

<?php

class RegisterController {
    public $userRepo;
    public $blogRepo;
    public $mailer;
    public $avatarRepo;

    public function __construct(UserRepository $userRepo, AvatarRepository $avatarRepo, BlogRepository $blogRepo, Mailer $mailer) {
        $this->userRepo = $userRepo;
        $this->blogRepo = $blogRepo;
        $this->mailer = $mailer;
        $this->avatarRepo = $avatarRepo;
    }

    public function register(Request $request) {
        $user = $this->userRepo->register($request);
        $this->avatarRepo->uploadFor($user, $request->input('base64_avatar'));
        $this->blogRepo->makeNewFor($user);
        $this->mailer->sendRegisteredConfirmationTo($user);
    }
}

It feels like my controller is doing too many things :

  • Creates a user
  • Upload the avatar and link it to the user
  • Create a blog and link it to the user
  • Send a mail to the user

But it also feels like my controller is only doing one thing here : "Registering a user".

In the same manner, what if the RegisterController now also has a unregister() method ? Is it going to violate the SRP ? Since one could say it'll then handle registering AND unregistering, but one could also say it'll still only handle the concept of registering.

Where is the line that says "the scope of this class or this method is too wide, there should be two classes/methods" ?

\$\endgroup\$
2
  • \$\begingroup\$ Is this actual, working code you use, or example code? \$\endgroup\$
    – Phrancis
    Apr 20, 2016 at 18:25
  • \$\begingroup\$ This is an example but it's coded the way I code in the real world. \$\endgroup\$ Apr 20, 2016 at 18:31

1 Answer 1

3
\$\begingroup\$

Am I violating the SRP in this code?

Not really. And if you are, it doesn't matter.

When a user registers, you want to perform 4 actions, which means that these 4 actions need to be applied in one place, and a register function is the right place for it.

Note that the original definition of "responsibility" in the context of SRP is "a reason for change". So you could look at it like this: The only responsibility of the register function is to register a user. You don't call it when a user unregisters, when a user changes their password, when a user needs a password reset, etc.

You also have separated the actual implementations in the appropriate classes, so I don't see a problem here.

Misc

  • does the register method really need the whole $request class? It should be the task of the controller to parse the user input, and only pass on needed stuff (eg password, username, etc).
  • is there a reason that your fields are public?
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your answer, I agree it doesn't need the whole $request class. No there's no reason that my fields are public, they'd be private in real life, since they shouldn't be used elsewhere. Any comment about an "unregister()" method ? Thinking your way, unregister() would violate the SRP since the RegisterController would change if the registering changed, or if the "unregistering" changes. Am I right ? \$\endgroup\$ Apr 20, 2016 at 19:04
  • \$\begingroup\$ @SteveChamaillard Well, I would say that the responsibility of a RegisterController is to controll the registered state of a user. So the reason for change is a change in the registered state of a user. If you would look at it the way you do, you couldn't even write a counter class that has an increase and decrease function. Anyways, the finer details of the SRP are probably off-topic here. You might want to try programmers.SE, it should definitely be on-topic there (but check first if a similar question doesn't exist already). \$\endgroup\$
    – tim
    Apr 20, 2016 at 19:16

Not the answer you're looking for? Browse other questions tagged or ask your own question.