Dealing with complexity in controllers

I've come across similar situations and I always end up creating a class or group of classes that take care of the process.

Things like iterating over the same collecion twice intead of once, or calculating a value more than once instead of storing it in a local variable are not going to be a performance hit. And you shouldn't worry about it if it results in more readable code and shorter functions.

You could make a class that's something like StudentAccountCreator with a createAccounts method that receives a collection of students and returns a collection of accounts.

Then in your controller you could do something like this:

    $accounts = DB::transaction(function() use ($students) {
        return $accountCreator->createAccounts($students);
    });
    
    $accounts->each(function($account) {
        Mail::queue(new AccountCreated($account));
    });

AccountCreated would be a Mailable. https://laravel.com/docs/5.5/mail#writing-mailables

Also, if you have to encrypt passwords to make accounts you're eventually going to hit a timeout if the list of students is too long. You'll have to either limit the max number of students in one request or move all of this to a Job and queue it.

Thank you @ftrillo, quite a few interesting things in your response. I'll keep the timeout issue in mind; not many students (around 30), but we'll see how the hardware performs.

The most interesting piece of information for me was (don't be dissapointed...) that the DB::transaction() method returns the result of the closure passed as its parameter. I was wondering how I could 1) collect some errors inside the closure and display them after the transaction is over (I thought of a parameter that is passed by reference to the callback) and 2) how would I generate a redirect from inside the callback, because I have to return a redirect() from the enclosing controller method, not from the callback!

But let's get back() to the original question :-) You are suggesting that I just move the code out of the controller to another class. This means that I will have a fat method but in another place - I just moved the complexity around. My question is - well, then when is it wrong to have a large method? What I'm interested in, I think, is some guideline to be able to detect by myself when I'm doing something wrong (even if I don't have the solution for the moment), so that I can stop and rethink.

I would move the validation to a request class, put the user creation logic in a static model method and fire an event (UsersWereCreated) to do anything else like send mail. Every class should ideally have one distinct responsibility. Request class manages the incoming request. Controller class decides what to do with requests. Model is responsible for modeling database records. UsersWereCreated event class manages event data. SendWelcomeEmail event listener sends email to new users. This means that you can very easily unit test every part of this process and hopefully keep them distinct from one another.

At least for me, when I find myself saying 'this class does A AND B AND C', then I know its time to consider a refactor. In some cases it makes sense to not refactor, but if your controller method is making you uncomfortable and is doing multiple tasks, its probably worth creating other classes to perform bits of your logic.

@kataklys

Yeah, transaction returning the value is pretty cool. That kind of little thing is not always covered by the documentation. I didn't find out until I got curious and checked the API docs here. https://laravel.com/api/5.5/

I see what you mean when you say that you'd only be moving a long function to a different place.But I was beign vague when it comes to implementation details.

Of course the createAccounts() function could just be a short one, that just iterates over the students and calls another function that takes care of creating a single account.

In my opinion having a large method is always worse than having multiple shorter methods. How long a method I'd tolerate would depend on a lot of things. Like, how much of the code I want to be able to reuse somewhere else, how likely the process is to change in the future, how long the code is going to be used... stuff like that.

Refactoring is a complex topic, and probably a bit beyond the scope of this forum. I learned about it in this book. Which I recommend to anyone interested. https://martinfowler.com/books/refactoring.html

@zachleigh

When you say "I would move the validation to a request class" you're referring to form requests, right? (https://laravel.com/docs/5.5/validation#form-request-validation) But would it really be ok if I do the validation for a User outside the User class? I'm thinking about the fact that maybe I create users in various parts of the app, and I want to reuse the validation code - but a form request is designed for a certain form (a certain mix of information which also happens to contain users), not for a certain model.

Also, why should I create a static model method for instantiating users, since the model already has its own create()/save()/newInstance() methods? Just to be able to fire that event?

@ftrillo

Regarding the API docs I have a long-term frustration, which I expressed in another post (your trick with the result of the transaction() method gave me the impulse).

Sorry I misinterpreted what you said. I was going to create separate methods anyway, as you clarified in the second post. I'll just have to be careful how I deal with exceptions, transactions and error message bags (multiple users, each having multiple fields, each having multiple validity constraints...)

@kataklys

When submitting a form, you would need to have the user information separated from any other information anyway.

[
    'users' => [
        [user 1 data],
        [user 2 data],
        etc.
    ],
    'something else' => [
        //
    ],
]

So in your form request the logic to validate the users information would be namespaced.

'user.*.email' => 'required',
'user.*.name' => 'required|unique',
etc.

So if you do end up making users from some other point in your app, you could simply put the user validation rules on a parent request class or in a trait and merge them in any other rules needed for the request.

The static method on the User model would encapsulate all the logic for starting the transaction, looping through the users and firing the event. This way, if you did end up creating users from some other point in the app, it would be as easy as User::createBulk($data).

Thank you @zachleigh , nice ideas regarding validation (yes, user information is namespaced indeed). Question is: if I put that static createBulk() method inside the User class, doesn't that mean that the User class is beginning to do too much? (you know, that single responsibility principle...) Where do I draw the line?

@kataklys The single responsability principle can be read as, 'every class should only have one reason to change'. Your User model will have to be changed if you alter your users table.

Adding a bulk insert method to the class, wont add any extra reasons to change the class, because It's still depending on the users table staying the same. At least that's how I see It.

If you really want it in separate places you should look into Repositories.

@kataklys

The User model is responsible for the users table database records so having a bulk create method on the class makes perfect sense to me.