class UserCreator
def create_user(email)
user = create_user_object(email)
assign_admin_role(user)
assign_invite_permission(user)
call_invite_template(user)
end
private
def create_user_object(email)
User.create(email)
end
def assign_admin_role(user)
user.roles << find_admin_role
end
def assign_invite_permission(user)
user.permissions << find_invite_permission
end
def call_invite_template(user)
find_invite_template.call(user: user)
end
def find_admin_role
Roles.find_by(name: 'admin')
end
def find_invite_permission
Permissions.find_by(name: 'invite')
end
def find_invite_template
Templates.find_by(name: 'invite')
end
end
Slightly less worse, but the original function that I inputted into ChatGPT to generate the code from my message was this:
def create_user(email)
user = User.create(email: email)
user.roles << Roles.find_by(name: 'admin')
user.permissions << Permissions.find_by(name: 'invite')
Templates.find_by(name: 'invite').call(user: user)
end
IMO this 4 line function is significantly better in terms of clarity, readability, and it avoids unnecessary state. Testability and encapsulation are the same.
(I would argue that the encapsulation is better with a function, since encapsulation is way too easy to break in Ruby, but hey, that's me)
I think the choice between a method (ruby doesn't have functions ;) ) and a class here may well be right. The only reason for using a class here is more as a general principle if you want it to be easier to pass your "usercreator" around as a value, but you can of course wrap it in a lambda if needed or use `#method`. Alternatively you can just make it a lambda from the start. If your main objection to the class is the ease of breaking encapsulation, lambda's are quite nice for tighter encapsulation of state in Ruby.
E.g.
lambda do
state = 0
->() { state += 1 }
end.call
(Note the `.call` at the end - we don't want the outer lambda; that is just used for creating the variables that will hold the state - we want the inner lambda)
So let's say that for some stupid reason your user creator object needs to keep a count of users, you could "seal" that state into a lambda with the above technique, and make it really hard (I'm not sure if impossible) to break encapsulation. E.g. instance_eval or trying obtain the binding doesn't work, because they both will get you the state of the Proc object containing the closure, not the local variables accessible within it.
It provides some very marginal improvement, but doesn't address the actual problem of the code, which is the horrific verbosity. The use of attributes is the least of my problem with it. This is how I'd want it to look:
class UserCreator
# Arguably, I'd prefer *call* because that allows it to be interchangeable with a lambda
# But frankly this thing could *be* a lambda. E.g. you could replace the above class declaration
# with "UserCreator = ->(email) do" and ditch the "def"
# The exception, where I'd allow for an initialize and attributes would be in cases where you'd otherwise
# be passing a *lot* of *the same* state around between multiple methods
#
def self.create_user(email)
User.create(email).tap do |user|
user.roles << Roles.find_by(name: 'admin')
user.permissions << Permissions.find_by(name: 'invite')
Templates.find_by(name: 'invite').call(user: user)
end
end
end
The whole service object pattern is heavily abused by people who don't seem to understand which (limited) situations it actually provides benefits. This isn't one of them.
Doesn't that already look 100x better?