4 ms·

The old code was targetEndDate = new Date(); targetEndDate.setFullYear(endDate.getFullYear()); targetEndDate.setMonth(endDate.getMonth()); targ

The old code was targetEndDate = new Date(); targetEndDate.setFullYear(endDate.getFullYear()); targetEndDate.setMonth(endDate.getMonth()); targetEndDate.setDate(endDate.getDate()); Even if the endDate is valid, this can fail, because today is the 31st, which doesn't exist in endDate's month. If you just did: targetEndDate = createDate(endDate.getFullYear(), endDate.getMonth(), endDate.getDate()); // createDate is an intentional placeholder, I didn't want to double-check the actual syntax you wouldn't pass through the transitional stage where you have the new month but today's day of the month, which is what causes the bug. In this case, using mutation on the individual fields makes you have to transition through an invalid state to get back to a valid one, and the JS date object does something unexpected (though I think there's nothing good to do here, an exception is probably the best you could do). So mutation really is at issue here. In general, mutation creates room for these kind of counter-intuitive state transitions to arise.

macintux 3y ago
I agree, reading the example code was painful as someone who tries to write all code in a functional style.
magicalhippo 3y ago
> mutation on the individual fields makes you have to transition through an invalid state If you want to do this mutation-style for some reason, you should use the builder pattern (or whatever it's called). So in this case you'd instantiate a DateBuilder instance instead of a Date instance, and finally call .date() to get the actual Date instance from the builder instance.
- jasomill 3y ago
  Better yet, in this particular case, don't use the builder pattern, and instead just provide a sensible set of constructors with appropriate defaults for unspecified arguments (e.g., 00:00 for unspecified time). For a simple date/time class, the builder pattern reeks of pointless overengineering.
  - magicalhippo 3y ago
    Oh certainly, didn't mean to imply it's great for a freaking date. But I've seen the mutating pattern in other cases causing the weird state bug which could have avoided using a builder.
rawling 3y ago
The direct "immutable" equivalent of the bug code (purely based on my experience with .NET Core immutable collections) would have the setX methods return a new immutable datetime with the field set as requested. So just "use an immutable type" wouldn't fix this bug. "Change the code completely so you supply all 3 components at the same time" obviously fixes the bug, but that doesn't require an immutable type.
- tantalor 3y ago
  Yep, that's what I was getting at. Immutability is not a "silver bullet"
- jasomill 3y ago
  The .NET immutable collections seem to mostly avert problems like this. For example, ImmutableDictionary.Add<TKey, TValue>(key, value) throws an exception if key already exists with a different value (as determined by an explicit or implicit IEqualityComparer<TValue>). .NET's (immutable) DateTime, on the other hand, has AddMonths(Int32), which sets the day to Min(original day number, last day of result month), which, while arguably reasonable, isn't obvious without reading the documentation. My favorite counterintutive DateTime "mutator," however is AddMilliseconds(Double): prior to .NET 7, its floating-point argument is rounded to the nearest integer (!?!).

macintux 3y ago

I agree, reading the example code was painful as someone who tries to write all code in a functional style.

magicalhippo 3y ago

> mutation on the individual fields makes you have to transition through an invalid state If you want to do this mutation-style for some reason, you should use the builder pattern (or whatever it's called). So in this case you'd instantiate a DateBuilder instance instead of a Date instance, and finally call .date() to get the actual Date instance from the builder instance.

jasomill 3y ago
Better yet, in this particular case, don't use the builder pattern, and instead just provide a sensible set of constructors with appropriate defaults for unspecified arguments (e.g., 00:00 for unspecified time). For a simple date/time class, the builder pattern reeks of pointless overengineering.
- magicalhippo 3y ago
  Oh certainly, didn't mean to imply it's great for a freaking date. But I've seen the mutating pattern in other cases causing the weird state bug which could have avoided using a builder.

rawling 3y ago

The direct "immutable" equivalent of the bug code (purely based on my experience with .NET Core immutable collections) would have the setX methods return a new immutable datetime with the field set as requested. So just "use an immutable type" wouldn't fix this bug. "Change the code completely so you supply all 3 components at the same time" obviously fixes the bug, but that doesn't require an immutable type.

tantalor 3y ago
Yep, that's what I was getting at. Immutability is not a "silver bullet"
jasomill 3y ago
The .NET immutable collections seem to mostly avert problems like this. For example, ImmutableDictionary.Add<TKey, TValue>(key, value) throws an exception if key already exists with a different value (as determined by an explicit or implicit IEqualityComparer<TValue>). .NET's (immutable) DateTime, on the other hand, has AddMonths(Int32), which sets the day to Min(original day number, last day of result month), which, while arguably reasonable, isn't obvious without reading the documentation. My favorite counterintutive DateTime "mutator," however is AddMilliseconds(Double): prior to .NET 7, its floating-point argument is rounded to the nearest integer (!?!).