6

This question already has an answer here:

Let's say I have a model of some data:

public class User {

    public String name;
    public String password;

}

And some other class to work with this User:

public class ClassB {

    public static void saveUser(User user) {
        // Something 
    }

    public static void saveUser(String username, String password) {
        // Something
    }

}

What is the better way to go? Would you better choose second method so not to couple these two classes or choose the first way just passing a User instance?

marked as duplicate by gnat, user53019, k3b, Kilian Foth, ozz Oct 2 '13 at 10:41

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 5
    if class B works with user let it work with user – ratchet freak Sep 30 '13 at 10:58
  • It works both ways, what's better approach in terms of future changes. – Eugene Sep 30 '13 at 11:01
  • If User contains a lot of data not needed by ClassB, I would not pass a User-instance to ClassB, but a subtype of User which contains only the needed data of User. – MrSmith42 Sep 30 '13 at 11:02
  • 9
    What's wrong with User.save()? – Izkata Sep 30 '13 at 13:55
6

Consider the following (some of it is from the excellent Clean Code by Uncle Bob):

If there are direct connection between the parameters, you should encapsulate it to a class. (something like that from the book)

Example: if you have a UserCredential class, you can easily add SSO or one-time-ticket based authentication. If you had a method or property called AreValid, you won't ever need to modify your code, even if you are adding those two new functionalities.

See?

public static void saveUser(UserCredentials credentials) 
{
    if (credentials.AreValid)
    {
        // Something - won't ever change
    }
}

Note: It is not even a User class, just the credentials.

The additional benefit of creating smaller classes (UserCredentials instead of User) is: you can add logic into that small class - so finding out if the user has valid credentials won't pollute your code where you are focusing on saving something: you are separating the responsibilities (check: Single Responsibility Principle).

  • Not to mention that according to Uncle Bob the ideal number of parameters for a method is none. One, if you must. Two, if you really must. And three, only if there is absolutely no other choice. :) – Dima Sep 30 '13 at 14:48
  • Yay @Dima, one of the rules people really hated when I was doing presentation from the book :). Of course, much can be done here, using ICredentials interface to decouple the implementation, etc. but that would be overkill for the question – atoth Sep 30 '13 at 14:57
13

As you've identified, saveUser(User user) couples the two classes. This is not always bad. If the sole purpose of class B is to save a User then it's fair enough for it to expect a User object to save. However, it does potentially lead to extensibility issues, if the model is complex enough.

It also gives the saveUser method access to any logic you add to your User class. A low risk, but a risk nontheless.

On the other hand, saveUser(String username, String password) carries its own problems. it is very easy, especially when you have 10 or more parameters, to get them mixed up. If someone accidentally types saveUser(user.Password, user.Username) then you're not going to get a compile-time error, or even a runtime error, it's going to happily save those strings in the wrong fields.

Also, a very long list of parameters can make your code unreadable.

A better option is probably to use an interface saveUser(IUserDetails user) which doesn't couple the classes to each other, but rather couples the saveUser method to any class that contains the data it requires to perform the task it has been given to do -- save a username and password.

  • 1
    Or IUserCredentials which could then be implemented by User directly or returned by one of its member-functions. – Supr Sep 30 '13 at 13:58
5

I would go for an object. First of all, its object-oriented but also because of readability:

User user = new User("aString", "anotherString")
saveUser(user);

is better readable than:

saveUser("aString", "anotherString");

and it's more safe, e.g.:

saveUser(password, username) //oops
4

It is rarely a good idea to use multiple parameters to represent a single "object" (here used as a logically cohesive set of variables) in a function, when you can it is advisable to only pass a single parameter for each such "object".

Consider that when you change the permission framework to use a authentication token then you'd need to change all calls that use username/password plus the implementation of those functions, compared to only passing user where you just need to change the implementation.

Or take a third option and create a sub object like an authentication token (to continue the analogy) which you can pass around, this is higher level than username/password strings but still decouples user from ClassB.

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