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?

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.

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.

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().

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()
    });
}