3

At first I had the following logic of user creation:

class UserService {
  constructor(userRepository, postRepository) {}

  createUser({user}) {
    this.userRepository.create(user);
  }
}

Then I also got another use case when while creating a user we also have to create a default post for him.
The new use case should be identical to the previous one, just additionally create a post. And I see two different ways of how it can be implemented:

  1. Create a separate method for new use case and inside of it call the user-creation use case.
class UserService {
  constructor(userRepository, postRepository) {}

  createUser({user}) {
    this.userRepository.create(user);
  }
  
  createUserWithDefaultPost({user, post}) {
    this.createUser({user});
    this.postRepository.create(post);
  }
}
  1. Just add a new condition for a situation when post data is passed:
class UserService {
  constructor(userRepository, postRepository) {}

  createUser({user, post}) {
    this.userRepository.create(user);

    if (post) {
      this.postRepository.create(post);
    }
  }
}

In my opinion, an approach with separate method creation looks better, and cleaner in some way, but that's just kind of my feelings and I can't give some clear explanation why it should be done.
Somehow it's definitely related to separation of concerns, but approach with "if" condition still looks pretty normal.

Improve this question
3
  • 2
    Please clarify your specific problem or provide additional details to highlight exactly what you need. As it's currently written, it's hard to tell exactly what you're asking. Commented Oct 12, 2023 at 23:43
  • 1
    Apples or bananas? That's pretty much what you're asking here. Both cases work. Why do you think that there should be one universally superior solution to this? Commented Oct 12, 2023 at 23:58
  • 1
    Might depend on whether there is a reasonable "default" post. If so, I prefer version 2. If not (which seems most likely here) I prefer separate methods, though I wouldn't call the 2nd one "createUserWithDefaultPost", I'd just have a normal createPost(user, post) and call it. Commented Oct 13, 2023 at 0:16

5 Answers 5

Reset to default
5

Never take the changing of a public methods signature lightly.

You didn't specify a language. But if yours doesn't have default arguments then 2 just broke the using code. Maybe you're lucky enough that createUser isn't very exposed so that's just a little refactoring but still, everything you touch needs testing again. Ick.

Maybe your code is all shiny and new and that's not an issue. Fine, You're also up against a classic code smell.

c2.com called it False Unification of Procedures

A procedure, function, or method has a boolean that provides a variation on its behavior, but the two variations in fact have completely different semantics. It would be better to refactor the common code into another method and split the remainder into two separate methods, removing the boolean.

c2.com Code Smell

Martin Fowler called it a Flag Argument:

My reasoning here is that the separate methods communicate more clearly what my intention is when I make the call. Instead of having to remember the meaning of the flag variable when I see book(martin, false) I can easily read regularBook(martin).

martinfowler.com Flag Argument

Robert Martin called them Boolean Arguments

Boolean arguments loudly declare that the function does more than one thing. They are confusing and should be eliminated.

informit.com Eliminate Boolean Arguments

Whatever you call 2, it has problems.

1 is the clear winner provided you have the one thing it demands: a good name. That's the cost you incur here. Understand that it's more than thinking up a name that makes what-this-function-is-for easy to remember (don't make me keep looking inside because the name is bad). It's also picking a name that leaves room for whatever else you're going to add here. Tacking the arguments on to a method name can get ugly. A good name is not about what happens inside. It's about why you'd want to call it.

Coming up with good names is hard. But if you have a good name, use it.

Improve this answer
5
  • 1
    post is not a boolean. At least how I read his code, it's probably a string or an object. The code looks like modern JavaScript, so no breakage of existing code. by adding the post option. However, I largely agree with your overall message, so I'm not downvoting. Commented Oct 13, 2023 at 4:54
  • @user949300 if we assume JavaScript then the default value of the post argument is falsy which avoids the signature issue. But it is still being run through a context where it is coerced into being a Boolean (falsy or truthy) and so the Boolean criticism stands. Commented Oct 13, 2023 at 12:00
  • Martin’s argument is not against flags. It is against flags named “true” and “false”. createUser(Martin, student) vs createUser (Martin, teacher) would be fine. In Swift, createUser(name: Martin, isStudent: true) would be fine. CreateUser(Martin, true) isn’t. Commented Feb 25, 2024 at 17:19
  • @gnasher729 it may have escaped your notice but this answer mentions two completely different Martins. Which one are you talking about? Commented Feb 25, 2024 at 19:52
  • @candied_orange Could have been worse. I know a Ryan Lee and a Lee Ryan personally:-) And Kent Clark’s outnumber Clark Kent’s eight to one. Or the other way round. Meant Martin Fowler. Commented Feb 25, 2024 at 22:21
4

Alternative:

class UserService {
  constructor(userRepository) {}

  createUser({user}) {
    this.userRepository.create(user);
  }
}

class PostService {
  constructor(postRepository) {}

  createPost({user}) {
    this.postRepository.create(post);
  }
}

class UserWithPostService {
  constructor(UserService, PostService) {}

  createUserUserWithPost({user, post}) {
    // Can start a transaction here for example
    const userId = this.UserService.createUser(user);
    this.PostService.createPost(post, userId);
  }
}

It separates out the responsibilities, re-uses the existing procedures. For example createUser can become more complicated like: generatePassword, sendWelcomeMail etc. It will all be re-used without additional complications.

Also your UserWithPostService can be extended with conditions like: Error when user is created with a subscription which doesn't allow to post.

Improve this answer
2

Why not this?:

class UserService {
  constructor(userRepository, postRepository) {}

  createUser({user}) {
    this.userRepository.create(user);
  }
  
  createUser({user, post}) {
    if (post is null) throw exception;
    this.createUser({user});
    this.postRepository.create(post);
  }
}

In languages like Java methods can have the same name as long as the parameter list is diffferent (method overloading) and this style seems to be standard. In the other hand, in Objective-C the first option, with verbose method name, seems to be the accepted standard.

Improve this answer
2

Always worth considering: An API change can break things. If you are developing an application, the effect can be minimal. I can change an API, press "build" and Xcode shows me what to change, so that is easily done. A change in a library used by 100 developers outside your company is mightily inconvenient for them. But that's not really the point.

The point is: How do you call this method? You have two choices. You can make calls

customer.create(name, false);
customer.create(name, true);

or you can make calls

customer.create(name);
customer.createWithPost(name);

Which is better? If the caller knows which one to use, and always the same, then the second is better. If the caller himself has a bool parameter "post" then it is quite inconvenient to write

if (post) {
    customer.createWithPost(name);
} else {
    customer.create(name);
}

And in many languages where you don't have argument names in a call, you have to guess what "true" and "false" mean, which is against the first method. In Swift where you would call

customer.create(name, withPost: true/false)

that's much less of a problem or no problem at all. And finally, how bad would it be for your developer to make either one or two calls? How do the developers view this: They create users with or without posts (so a user created without posts would permanently be without posts, or at least unless the do something in the UI to request a post), or is this a utility, where you figured that you always create users with posts and want to save a function call?

Improve this answer
0

There are additional options besides the described two. When the concern is loose coupling observer design pattern is a possible solution. Enhance the UserService with methods to register/withdraw observers, update its createUser method to notify observers about the created user, develop new service that creates the post and register it to UserService so it gets notified when a user is created.

Improve this answer
1
  • that's a really interesting approach as well, thanks. will read more about it Commented Jun 19, 2024 at 12:45

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.