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

ccoding-stylejavamethod-chaining

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?

Best Answer

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)

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
That predictPrice didn't modify Item, but returned the predicted price
That predictPrice was a void method called setPredictedPrice
That predictPrice returned this (for method chaining to other methods on whatever instance it's a part of) (and was called setPredictedPrice)

Related Solutions

Java – Why did memory-managed languages like Java, Javascript, and C# retain the `new` keyword

Your observations are correct. C++ is a complicated beast, and the new keyword was used to distinguish between something that needed delete later and something that would be automatically reclaimed. In Java and C#, they dropped the delete keyword because the garbage collector would take care of it for you.

The problem then is why did they keep the new keyword? Without talking to the people who wrote the language it's kind of difficult to answer. My best guesses are listed below:

It was semantically correct. If you were familiar with C++, you knew that the new keyword creates an object on the heap. So, why change expected behavior?
It calls attention to the fact that you are instantiating an object rather than calling a method. With Microsoft code style recommendations, method names start with capital letters so there can be confusion.

Ruby is somewhere in between Python and Java/C# in it's use of new. Basically you instantiate an object like this:

f = Foo.new()

It's not a keyword, it's a static method for the class. What that means is that if you want a singleton, you can override the default implementation of new() to return the same instance every time. It's not necessarily recommended, but it's possible.

C++ – Good Design Pattern for a Wrapper Around a C Object

If the responsibility for cleaning up dynamically allocated stuff shifts between your wrapper class and the C library based on how things get used, you are either dealing with a badly designed C library, or you are trying to do too much in your wrapper class.

In the first case, all you can do is keep track of who is responsible for the cleanup and hope no mistakes are made (either by you or by the maintainers of the C library).

In the second case, you should rethink your wrapper design. Does all the functionality belong together in the same class, or can it be split into multiple classes. Perhaps the C library uses something similar to the Facade design pattern and you should keep a similar structure in your C++ wrapper.

In any case, even if the C library is responsible for cleaning up some stuff, there is nothing wrong with keeping a reference/pointer to that stuff. You just need to remember that you are not responsible for cleaning up what the pointer refers to.

Best Answer

Related Solutions

Java – Why did memory-managed languages like Java, Javascript, and C# retain the `new` keyword

C++ – Good Design Pattern for a Wrapper Around a C Object

Related Topic