Introduced 'added' and 'subtracted' functions

[magwo]

Introduced 'added' and 'subtracted' functions on moment prototype, for adding and subtracting time but returning a modified object copy, leaving the original moment object intact. Also made a short, simple unit test and updated the docs. Did not commit build generated files.

[timrwood]

Isn't this just the following?

moment().clone().add()
moment().clone().subtract()

I think it may be confusing to have both an add and added methods, with the only thing changing being that one is cloned and the other is not.

[magwo]

Yes, I see your point. However I think that programming with immutable types is a good idea, and code with immutable types has a much lower bug frequency. These 2 functions encourage keeping the moment objects immutable by providing a clear and short expression. I think that many API users expect something like these functions to exist, and I believe it increases the readability of the code.

Personally I would prefer to only have the immutable versions of these functions, but that would break existing code and also possibly present a performance problem in some scenarios. In a purist stance, I believe it is a design mistake to have a datetime type mutable.

[icambron]

FWIW, I was also surprised at having to do moment.clone().add(), and would prefer that the obvious methods cloned internally (though I'm not sure I'd go as far as making it fully immutable). However, I don't think the names added and subtracted are intuitive. I don't have better name suggestions, though (in Ruby, you'd have add and add!, but AFAIK, there's no equivalent convention in JS).

[timrwood]

Native dates in javascript are mutable.

new Date().setHours(1);

Wouldn't it stand to reason that a wrapper around native dates is also mutable?

[icambron]

I'd suggest one of the nice things a wrapper could do is provide an immutable interface to the native mutable data type.

[timrwood]

I am hesitant to do this because of a couple reasons.

1 Performance decrease

While it's not a huge decrease, it is around 25-35% slower to switch to immutable. This is because we have to clone the moment any time we want a modification. This will also increase memory usage as well.

2 No way for developers to use mutable moments

Right now, moments are mutable with the option to clone, leaving the first moment untouched. If we force cloning, there is no longer an option. All manipulation methods will clone.

Perhaps a better solution would be to make it clear in the docs that when you use add, subtract, year, month, etc, you are modifying the original moment, and if you just want a copy of the moment with some changes, you should clone it first.

[icambron]

You'd have to define mutable versions of the methods regardless, at least for use by the library internally, at which point, you might as well expose them anyway. Blue sky, I'd have gone with add and addX, where add is clone().addX(), but that seems like a really confusing change to make now.

So as much as I like functional methods to be the "obvious" ones (it would spare my code base about a hundred clones), I think you're probably right that it's just best to tell users to clone everything themselves.

[magwo]

One argument from my side against the performance aspect, is that I use clone() a lot - I prefer to not modify existing objects whenever possible, and so I wouldn't experience much of a performance decrease.

I do agree that this change makes the API slightly more confusing. The "good" thing about this though, is that it forces the API users to think about the fairly important aspect of object mutability.

timrwood: Don't feel bad if you decide to reject this pull request, it's ok.

[timrwood]

Yeah, I think the problem is getting people to think about whether or not to clone, and as long as they think about it, it's just as easy to use .clone().add() as .added().

I'll add documentation on this, so hopefully that will help.