3 ms·
The original version isn't a code smell. It has a very important advantage: read the function and you know that a user can be only one of the two things, a "cu
by yongjik 4y ago
The original version isn't a code smell. It has a very important advantage: read the function and you know that a user can be only one of the two things, a "customer" or an "admin", and you never have to worry about any other cases, because they don't exist.
Make it a callback, and now the code becomes "Here we do something specific to each subclass of users, but this code doesn't tell you what they are, there can be any number of classes and the callback can literally do anything, and if you want to figure out what's happening at this particular step, scour the entire codebase and find all the callbacks that are passed as arguments."
The code has become extensible, and thus harder to reason about. This is a good trade-off if you know you will have more and more user roles, added by different devs (or even different teams), and you want the user creation code to be decoupled from roles.
On the other hand, if most of your business logic is "Do this for customers" and "Do that for admins," then it's a useless extensibility. You will need a major change of your business requirement before a callback would be useful.
YAGNI.
- rcme 4y ago“Do this for customers” and “do this for admins” has to be the number one regret of people who implement systems requiring access controls. Using roles and privileges is a much better approach.