Java return method good practice when filling an object

Question

Sorry for the ambiguous title. I'll edit when if someone has a better way to explain this.

Let's say I have a class that I create many instance of and that has many attributes that get filled by different methods. Is it bad practice to pass the object around and fill the attributes in the methods ? I'll try to explain with an example that I tried to make simple:

public class User {
    private String surname;
    private String name;

    public String getSurname() {
    return surname;
    }

    public void setSurname( String surname ) {
        this.surname = surname;
    }

    public String getName() {
        return name;
    }

    public void setName( String name ) {
        this.name = name;
    }

}
// Passing the object as parameter and returning the object in each methods
// In this case getNameFromSomewhere returns a User object

public User getUser(){ //edit: my mistake here
    User user= new User();  

    user = getNameFromSomewhere(user);  
    user = getSurnameFromSomewhere(user);    

    return user;    
}

In my case, getNameFromSomewhere does a search on a server, I was wondering if I should change all my methods so it returns a string just like the attribute and then just do :

// Alternative ?
public User getUser(){ //edit: my mistake here
    User user= new User();  

    user.setName(getNameFromSomewhere());   // getNameFromSomewhere  return string 
    user.setName(getSurnameFromSomewhere());    

    return user;    
}

*note: I have string, int, list attributes to fill. edit: I wrote an alternative, I'm simply wondering, performance wise, if it's good to pass a User as parameter and then return it filled with 1 attribute for every attribute or if I should just use the User.set method to fill the attribute and have my methods return the attribute type. (is this a bit more clear?)

Answer 1

The problem with your code is that User class exposes its internals through the setter methods, breaking the information hiding principle. Your approach could lead to an unmaintainable code base, i.e. it will be difficult to trace all the components that could modify a User object.

I think a better approach is to have a constructor that takes directly the information needed to build a User.

public class User {
    private String surname;
    private String name;

    public User(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }

    public String getSurname() {
        return surname;
    }

    public String getName() {
        return name;
    }
}

Then, you can build a user in the following way:

public User getUser() {
    User user = new User(getNameFromSomewhere(), 
                         getSurnameFromSomewhere());  
    return user;    
}

In this way you're sure where your user came from and that it cannot be modified anywhere else. Moreover, this code is compliant to the single responsibility principle, because the methods getNameFromSomewhere and getSurnameFromSomewhere have the only responsibility to retrieve the name / surname.

The optimal approach should be the one that use an immutable implementation of the User class. That means that every time you need to modify an object, you create a copy from it with the desired information changed. This way the whole testing process becomes simpler.

Answer 2

yes, it is better to have getNameFromSomewhere() return the value instead of having it accept a user Object and call the setter. There are two reasons for that:

1. You want to decouple the interface that User class exposes. so when you refactor surname into familyname, you dont need to change getNameFromSomewhere()
2. it is possible that getNameFromSomewhere() will be used to fill attribute of some other bean, besides User.