Coding style issue: Should we have functions which take a parameter, modify it, and then RETURN that parameter?

Question

I'm having a bit of a debate with my friend over whether these two practices are merely two sides of the same coin, or whether one is genuinely better.

We have a function which takes a parameter, fills out a member of it, and then returns it:

Item predictPrice(Item item)

I believe that as it works on the same object that is passed in, there is no point going on to return the item. In fact, if anything, from the perspective of the caller, it confuses matters as you could expect it to return a new item which it doesn't.

He claims that it makes no difference, and it wouldn't even matter if it did create a new Item and return that. I strongly disagree, for the following reasons:

• If you have multiple references to the item passes in (or pointers or whatever), it allocating a new object and returning it is of material importance as those references will be incorrect.

• In non-memory managed languages, the function allocating a new instance claims ownership of the memory, and thus we would have to implement a cleanup method which is called at some point.

• Allocating on the heap is potentially expensive, and therefore it is important to whether the called function does that.

Hence, I believe it to be very important to be able to see via a methods signature whether it modifies an object, or allocates a new one. As a result, I believe that as the function merely modifies the object passed in, the signature should be:

void predictPrice(Item item)

In every codebase (admittedly C and C++ codebases, not Java which is the language we are working in) I have worked with, the above style has essentially been adhered to, and kept to by considerably more experienced programmers. He claims that as my sample size of codebases and colleagues is small out of all the possible codebases and colleagues, and thus my experience is no true indicator over whether one is superior.

So, any thoughts?

Answer 1

This really is a matter of opinion, but for what it's worth, I find it misleading to return the modified item if the item is modified in place. Separately, if predictPrice is going to modify the item, it should have a name that indicates it's going to do that (like setPredictPrice or some such).

I would prefer (in order)

1. That predictPrice was a method of Item (modulo a good reason for it not to be, which there could well be in your overall design) which either

   • Returned the predicted price, or

   • Had a name like setPredictedPrice instead

2. That predictPrice didn't modify Item, but returned the predicted price

3. That predictPrice was a void method called setPredictedPrice

4. That predictPrice returned this (for method chaining to other methods on whatever instance it's a part of) (and was called setPredictedPrice)

Answer 2

Within an object oriented design paradigm, things shouldn't be modifying objects outside of the object itself. Any changes to the state of an object should be done through methods on the object.

Thus, void predictPrice(Item item) as a member function of some other class is wrong. Could have been acceptable in the days of C, but for Java and C++, the modifications of an object imply a deeper coupling to the object that would likely lead to other design issues down the road (when you refactor the class and change its fields, now that 'predictPrice' needs to be changed to in some other file.

Returning back a new object, doesn't have associated side effects, the parameter passed in is not changed. You (the predictPrice method) don't know where else that parameter is used. Is Item a key to a hash somewhere? did you change its hash code in doing this? Is someone else holding on to it expecting it not to change?

These design issues are ones that strongly suggest that you shouldn't be modifying objects (I'd argue for immutability in many cases), and if you do, the changes to the state should be contained and controlled by the object itself rather than something else outside of the class.

---

Lets look at what happens if one fiddles with the fields of something in a hash. Lets take some code:

import java.util.*;

public class Main {
    public static void main (String[] args) {
        Set set = new HashSet();
        Data d = new Data(1,"foo");
        set.add(d);
        set.add(new Data(2,"bar"));
        System.out.println(set.contains(d));
        d.field1 = 2;
        System.out.println(set.contains(d));
    }

    public static class Data {
        int field1;
        String field2;

        Data(int f1, String f2) {
            field1 = f1;
            field2 = f2;
        }

        public int hashCode() {
            return field2.hashCode() + field1;
        }

        public boolean equals(Object o) {
            if(!(o instanceof Data)) return false;
            Data od = (Data)o;
            return od.field1 == this.field1 && od.field2.equals(this.field2);

        }
    }
}

^ideone

And I'll admit that this isn't the greatest code (directly accessing fields), but it serves its purpose of demonstrating an issue with mutable data being used as a key to a HashMap, or in this case, just being put into a HashSet.

The output of this code is:

true
false

What has happened is that the hashCode used when it was inserted is where the object is in the hash. Changing the values used to compute the hashCode doesn't recompute the hash itself. This is a danger of putting any mutable object as a key to a hash.

Thus, going back to the original aspect of the question, the method being called doesn't "know" how the object it is getting as a parameter is being used. Providing the "lets mutate the object" as the only way to do this means that there are a number of subtle bugs that can creep in. _{^{Like losing values in a hash... unless you add more and the hash gets rehashed - trust me, thats a nasty bug to hunt down (I lost the value until I add 20 more items to the hashMap and then suddenly it shows up again).}}

• Unless there is very good reason to modify the object, returning a new object is the safest thing to do.
• When there is a good reason to modify the object, that modification should be done by the object itself (invoking methods) rather than through some external function that can twiddle its fields.
  • This allows the object to be refactored with a lower maintenance cost.
  • This allows the object to make sure that the values that compute its hashcode don't change (or are not part of its hashcode calculation)

Related: Overwriting and returning the value of the argument used as conditional of an if statement, inside the same if statement

Answer 3

I always follow the practice of not mutating someone else's object. That is to say, only mutating objects owned by the class/struct/whathaveyou that you're inside.

Given the following data class:

class Item {
    public double price = 0;
}

This is OK:

class Foo {
    private Item bar = new Item();

    public void predictPrice() {
        bar.price = bar.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
foo.predictPrice();
System.out.println(foo.bar.price);
// Since I called predictPrice on Foo, which takes and returns nothing, it's
// apparent something might've changed

And this is very clear:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public double predictPrice(Item item) {
        return item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
foo.bar.price = baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// Since I explicitly set foo.bar.price to the return value of predictPrice, it's
// obvious foo.bar.price might've changed

But this is far too muddy and mysterious for my tastes:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public void predictPrice(Item item) {
        item.price = item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// In my head, it doesn't appear that to foo, bar, or price changed. It seems to
// me that, if anything, I've placed a predicted price into baz that's based on
// the value of foo.bar.price

Answer 4

When coding in Java, one should try to make the usage of each object of mutable type match one of two patterns:

1. Exactly one entity is considered the "owner" of the mutable object and regards that object's state as part of its own. Other entities may hold references, but should regard the reference as identifying an object which is owned by someone else.

2. Despite the object being mutable, nobody who holds a reference to it is allowed to mutate it, thus making the instance effectively immutable (note that because of quirks in Java's memory model, there's no good way to make an effectively-immutable object have thread-safe immutable semantics without adding an extra level of indirection to each access). Any entity which encapsulates state using a reference to such an object and wants to change the encapsulated state must create a new object which contains the proper state.

I don't like to have methods mutate the underlying object and return a reference, since they give the appearance of fitting the second pattern above. There are a few cases where the approach may be helpful, but code which is going to mutate objects should look like it's going to do so; code like myThing = myThing.xyz(q); looks a lot less like it mutates the object identified by myThing than would simply myThing.xyz(q);.

Answer 5

While I have not a strong preference, I would vote that returning the object leads to better flexibility for an API.

Note that it has sense only while the method has freedom to return other object. If your javadoc says @returns the same value of parameter a then it is useless. But if your javadoc says @return an instance that holds the same data than parameter a, then returning the same instance or another one is an implementation detail.

For example, in my current project (Java EE) I have a business layer; to store in DB(and assign an automatic ID) an Employee, say, an employee I have something like

 public Employee createEmployee(Employee employeeData) {
   this.entityManager.persist(employeeData);
   return this.employeeData;
 }

Of course, no difference with this implementation because JPA returns the same object after assigning the Id. Now, if I switched to JDBC

 public Employee createEmployee(Employee employeeData) {
   PreparedStatement ps = this.connection.prepareStatement("INSERT INTO EMPLOYEE(name, surname, data) VALUES(?, ?, ?)");
   ... // set parameters
   ps.execute();
   int id = getIdFromGeneratedKeys(ps);
   ??????
 }

Now, at ????? I have three options.

1. Define a setter for Id, and I'll be returning the same object. Ugly, because setId will be available everywhere.

2. Do some heavy reflection work and set the id in the Employee object. Dirty, but at least it is limited to the createEmployee record.

3. Provide a constructor that copies the original Employee and accepts also the id to set.

4. Retrieve the object from DB (maybe needed if some field values are calculated in the DB, or if you wish to populate a realtionship).

Now, for 1. and 2. you may be returning the same instance, but for 3. and 4. you will be returning new instances. If from the principe you stablished in your API that the "real" value will be the one returned by the method, you have more freedom.

The only real argument I can think against that is that, using implementatios 1. or 2., people using your API may overlook the assignation of the result and their code would break if you change to 3. or 4. implementations).

Anyway, as you can see, my preference is based in other personal preferences (like forbidding a setter for Ids), so maybe it won't apply to everyone.

Answer 6

Syntax is different between different languages, and it is meaningless to show a piece of code that is not specific to a language because it means completely different things in different languages.

In Java, Item is a reference type -- it is the type of pointers to objects that are instances of Item. In C++, this type is written as Item * (syntax for the same thing is different between languages). So Item predictPrice(Item item) in Java is equivalent to Item *predictPrice(Item *item) in C++. In both cases, it is a function (or method) that takes a pointer to an object, and returns a pointer to an object.

In C++, Item (without anything else) is an object type (assuming Item is the name of a class). A value of this type "is" an object, and Item predictPrice(Item item) declares a function which takes an object and returns an object. Since & is not used, it is passed and returned by value. That means the object is copied when passed, and copied when returned. There is no equivalent in Java because Java does not have object types.

So which is it? Many of the things you are asking in the question (e.g. "I believe that as it works on the same object that is passed in ...") depends on understanding exactly what is being passed and returned here.

Answer 7

The convention I've seen codebases adhere to is to prefer a style that is clear from the syntax. If the function changes value, prefer pass by pointer

void predictPrice(Item * const item);

This style results in:

• Calling it you see:

  predictPrice(&my_item);

and you know it will be modified, by your adherence to style.

• Otherwise prefer pass by const ref or value when you don't want the function to modify the instance.