Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
6  
Why isn't this function a property of the object? –  Florian Margaine Apr 10 '13 at 18:25
    
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 –  Juan Mendes Apr 10 '13 at 18:34
1  
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

4 Answers 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.

share|improve this answer
1  
Ah, yes. Forgot about method chaining. –  Robert Harvey Apr 10 '13 at 18:36
1  
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. –  Juan Mendes Apr 10 '13 at 18:37

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.

share|improve this answer
    
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 . –  Juan Mendes Apr 10 '13 at 18:42

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

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

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()
    });
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.