Search code examples
javaimmutability

Which one is the right way, changing values in void method or returning new Object?

Which one do you prefer when you need to modify a map/list or object, which is a better approach?, I mean in the perspective of;

  • maintenance
    • Any maintenance difficulties while dealing with void methods or new object creations?
  • cost
    • Yes still new object creations are in the function scope and will be available to GC but wondering how it affects the application If we do this in the whole project, could be great to hear from some experienced people
  • testability
    • Using void methods are hard to test rather in the calling method but could be great to hear ideas on this void vs new
  • robustness
    • Using a new approach seems like avoids side effects and more robust. What is your idea?

Let's say that I have a map that returned from the method;

public Map<Integer, User> getUsers() {
    Map<Integer, User> names = getNames();
    return names;
}

So in the same function, I need to add prefixes to them;

public Map<Integer, User> getUsers() {
    Map<Integer, User> names = getNames();
    addPrefixes(names);
    return names;
}

In the immutability perspective, which one is better? Should I use this way;

public Map<Integer, User> getUsers() {
    Map<Integer, User> names = getNames();
    Map<Integer, User> prefixedNames = getPrefixed(names);
    return prefixedNames;
}

If so, how to implement getPrefixed function, deep copy every User and add them to the new Map and return it, can you give an example?

Solution

  • Let's take a look at the code:

    public Map<Integer, User> getUsers() {
    Map<Integer, User> names = getNames();
    // addPrefixes or names=getPrefixes
    return names;
}

    getNames is a method from the same class ad getUsers. If getNames is public, it should not expose an internal field of the class. I mean, if you have something like:

    public Map<Integer, User>() getNames() { return this.names; }

    It's time to make a copy, since the state of your object may be corrupted by any user. But even if getNames is a private and returns an internal field, what happens if someone calls getUsers (once, twice, ...)? The prefixes will be added to the internal field.

    Hence, I assume that getNames returns a copy of the names (e.g. retrieved from a database). The local variable names is yours and is a work copy of the names.

    Now, you have three options:

    void addPrefixes(Map<Integer, User> names) { // 1
    // update names
}

Map<Integer, User> getPrefixed(Map<Integer, User> names) { // 2
    // update names
    return names;
}

Map<Integer, User> getPrefixed(Map<Integer, User> names) { // 3
    // create copy, e.g. use Java8 stream;
    return namesCopy;
}

    The options 1 and 2 are basically the same (it's a matter of signature) whereas option 3 is a functional version that makes copy.

    Maintenance

    I think the option 2 and 3 are easier to read: give an input and get an output. The option 1 is harder to grasp: addPrefixes to what? To the names or to the object itself? Are there other side effects (now or later, in an improved version)?

    Plus, if the process of adding a prefix gets more complex, you can easily replace:

    return getPrefixed(names);

    By:

    return prefixer.getPrefixed(names);

    Cost

    You can start with option 3 (safer) and switch to option 2 (with comments) if the function is a bottleneck of your application. Option 1 and 2 have the same cost.

    Testabilty

    If you need to test if the prefixes are correctly added, make getPrefixed a public method of a small Prefixer class (Single Responsibility Principle).

    Robustness

    The option 3 is the best. What happens if getNames is modified to return an internal field. Example:

    // before
public Map<Integer, User>() getNames() { 
    // query the db
    // create the map
    return names; 
}

// after: the method was to slow, cache the result
public Map<Integer, User>() getNames() { 
    if (this.names == null) {
        // query the db
        // create the map
    }
    return this.names; 
}

    Option 1 and 2 will corrupt the field names.

    Important: whatever you decide, document your choice: when a parameter is modified, you should let the user know because the initial expectation (in Java) is that parameters are not modified. If a function returns a copy, let the user know.