Function that modifies an argument, should I return the modified object?

Question

We have a function that modifies a JS object, by adding some custom properties to it. The function doesn't return antyhing

addTransaction: function (obj) {
 obj.transactionId = this.getTransactionId;
 obj.id = this.recordId;
},

Somebody said they preferred that addTransaction return the obj.

Here's what I thought

• If I don't return anything (and document that the object is going to be modified), it's kind of clear that the object is going to be modified, as if the name were addTransactionToObj

• If I do want to add a return value, I shouldn't modify the incoming object, I should clone the given object, add my properties to the clone and return the clone.

• Having a return value that just returns one of the (modified) parameter just sounds wrong

Does anybody have a preference in this matter?

• Florian Margaine: Why isn't this function a property of the object?
• Ruan Mendes: Because the object is just a JS object that will be used to generate the values to send in the HTTP request. I could create another class , but it would have to take the transactionId and id as params, so it's more code everywhere that calls it. Seems like overkill
• TMN: Strongly disagree that "it's kind of clear that the object is going to be modified". That function name is too vague.

Answer 1

It allows you to do method chaining, which a lot of people feel improves readability. It's a very common idiom in JavaScript, which means a lot of people expect it, especially if the rest of the code base is similar. Your second point about cloning the object also has merit, if used to make your object immutable. How beneficial that is depends on your specific application.

• Robert Harvey: Ah, yes. Forgot about method chaining.
• Ruan Mendes: This is NOT method chaining, method chaining is when you return this or at least an object with methods on it, and you can take more actions on it, you can only call Object methods with the return value.

Answer 2

First of all, I think this function is strange... I mean that I expected the addTransaction to add a transaction to this, not the other way around. But maybe it's just me (edit: apparently not). If this is really what you want to do, then I suggest reading from "naming your function".

Secondly, I think that the main problem lies within the "add" part of "addTransaction". For me, it means that an object is going to be modified in order to have a "transaction".

Thirdly, there are multiple ways you can do that. You can:

• return a copy, which means your original object is immutable
• return the object to which the transaction is attached, so you're kinda following the fluent interface paradigm.
• return nothing, the object is modified

There is no "best" way, just be consistent and KISS when choosing for a given application.

Naming your function

Robert C. Martin's in (Clean Code tips on naming functions) states that the smaller the scope the more precise the name of the function should be. I suggest to name it to something along the lines of "attachTransactionIdAndRecordIdToObject" if you want to keep both setters in the same function. Again, whether you return the object itself or not is your choice.

• Ruan Mendes: It's a static method, it's just a convenience, the name could be clearer by calling it addTransactionToParamsObject, but the documentation of the object param makes it clear that it's modifying the passed in object. I do see your point that addTransaction .

Answer 3

If I don't return anything, it's kind of clear that the object is going to be modified

Not necessarily. It could be a method that causes side effects, like a logger.

If I do want to add a return value, I shouldn't modify the incoming object, I should clone the given object, add my properties to the clone and return the clone.

In general, I would agree with that. But returning the original object is a valid technique, especially if you're building a Fluent Interface.

As Florian points out, a better approach might be to add the function to the object itself, as in DoSomethingToThis().

• Ruan Mendes: this.transactionId and this.id are defined in the original object, I'd be forced to pass those to obj.addTransaction(this.transactionId, this.recordId)
• Ruan Mendes: Thanks for the feedback, can you provide some feedback on the solution I came up with?

Answer 4

I came up with something trying to incorporate all the suggestions. Make a class out of the object you're going to send with the AJAX request.

function MyAjaxParams() { 
 this.obj = {};
}
/**
 * @param {MyDocument} doc
 */
MyAjaxParams.prototype.addTransaction = function(doc) {
 this.transactionId = doc.getTransactionId();
 this.id = this.getRecordId();
}
MyAjaxParams.prototype.getParamsObject = function() {
 return this.obj;
}
// Many other methods to insert all required AJAX parameters

Then my existing code would do the following

MyDocument.prototype.save = function () {
 var ajaxParams = new MyAjaxParams();
 ajaxParams.addTransactionParams(this);
 ...
 Ext.Ajax.request.send({
 url: '/here/we/go',
 params: ajaxParams.getParamsObject()
 });
}