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?

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 — Ruan Mendes, Apr 10 '13 at 18:34
Strongly disagree that "it's kind of clear that the object is going to be modified". That function name is too vague. — TMN, Apr 10 '13 at 19:13

score 4 · Answer 1 · answered Apr 10 '13 at 18:35

4

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.

answered Apr 10 '13 at 18:35

Karl Bielefeldt

146,727
38
279
479

1

Ah, yes. Forgot about method chaining. – Robert Harvey Apr 10 '13 at 18:36
3

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. – Ruan Mendes Apr 10 '13 at 18:37

score 2 · Answer 2 · answered Apr 10 '13 at 18:40

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.

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` . — Ruan Mendes, Apr 10 '13 at 18:42

score 1 · Answer 3 · answered Apr 10 '13 at 18:34

1

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

answered Apr 10 '13 at 18:34

Robert Harvey

198,589
55
464
673

`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 Apr 10 '13 at 18:36
Thanks for the feedback, can you provide some feedback on the solution I came up with? – Ruan Mendes Apr 10 '13 at 18:57

score 0 · Answer 4 · edited Apr 12 '13 at 05:56

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

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

4 Answers4