Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add _.transform method #1901

Merged
merged 1 commit into from
Dec 29, 2014
Merged

Add _.transform method #1901

merged 1 commit into from
Dec 29, 2014

Conversation

jamiebuilds
Copy link
Contributor

_.transform is a useful alternative to _.reduce that calls the iterator with accumulator object each time and then returns it.

This is useful for creating new objects applying transformations of another object.

_.transform([1, 2, 3, 4], function(accumulator, value, index, original) {
  arr[index] = value * 2;
});
// >> [2, 4, 6, 8];
_.chain({ foo: 'foo', bar: 2, baz: 3 })
 .values()
 .filter(_.isNumber)
 .transform((arr, val, i) => arr[i] = val * 2)
 .value();
// >> [4, 6];

There was a previous issue on a similar function with the same name, however I think lodash has fully fleshed it out and is much more useful in this form.

I've taken the lodash implementation and tried to make it consistent with some of underscore's behavior.

  • No Object.create for obj.constructor
  • Can't return false to exit

Ref: @megawac marionettejs/backbone.marionette#2009 (comment)

@megawac
Copy link
Collaborator

megawac commented Oct 22, 2014

👍 to the addition, I'll take a peek at the implementation later tonight

@jashkenas
Copy link
Owner

To be clear — one would want to use this over reduce, simply because you don't have to bother to return accumulator here?

@megawac
Copy link
Collaborator

megawac commented Oct 22, 2014

@jashkenas, my personal favourite example is for deep mapping that respects both objects and arrays. The fact that it decides what your accumulator is based on the passed type is very useful

// From **http://stackoverflow.com/a/25334280/1517919**
function deepMap(obj, iterator, context) {
    return _.transform(obj, function(result, val, key) {
        result[key] = _.isObject(val) /*&& !_.isDate(val)*/ ?
                            deepMap(val, iterator, context) :
                            iterator.call(context, val, key, obj);
    });
}

@jdalton
Copy link
Contributor

jdalton commented Oct 22, 2014

I dig it because of the extra sugar around accumulators; inferring a default accumulator based on the kind of value iterated over and the ability to exit early once a result is reduced instead of having to do something like return result || (result = doSomething(...));.

That said, I have done some functional-fu like
_.uniq(_.transform(varDependencyMap, _.bind(push.apply, push), [])) with it too.

@jamiebuilds
Copy link
Contributor Author

and the ability to exit early once a result is reduced instead of being forced to do somehting like return result || (result = doSomething(...));.

I do think this would make the _.transform function much more useful. I omitted it initially because I didn't see any precedents in underscore for this type of behavior. I can easily add it back in though.

@jdalton
Copy link
Contributor

jdalton commented Oct 22, 2014

I do think this would make the _.transform function much more useful. I omitted it initially because I didn't see any precedents in underscore [snip]

It's being discussed on #1545.

@jamiebuilds
Copy link
Contributor Author

@jdalton Cool. If that change is made, I think it definitely makes the _.transform more appealing as you can't infer that you want to exit _.reduce when false is returned, as it is a valid next memo.

// **Transform** is an alternative to reduce that transforms `obj` to a new
// `accumulator` object.
_.transform = function(obj, iteratee, accumulator, context) {
iteratee = optimizeCb(iteratee, context, 4);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (obj == null) return/throw/???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@megawac
Copy link
Collaborator

megawac commented Oct 23, 2014

I think this kind of goes hand in hand with #1734

@megawac
Copy link
Collaborator

megawac commented Oct 23, 2014

LGTM big +1

// **Transform** is an alternative to reduce that transforms `obj` to a new
// `accumulator` object.
_.transform = function(obj, iteratee, accumulator, context) {
if (obj == null) return accumulator == null ? accumulator : {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have a test as well? Right now, it's returning accumulator if it == null, and an empty object if it does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely, I was trying to keep the same behavior that it'd have if you let the function run through, which is what the lodash implementation does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue being _.transform(null) === undefined and _.transform(null, function() {}, null) === null, where lodash returns a blank object. I think you're ternary is reversed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, derp. Fixed and added tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I wasn't clear with my initial comment.

@jdalton
Copy link
Contributor

jdalton commented Oct 23, 2014

This may be a good time to look at implementing _.create too as it could be used in _.transform, _.bind, _.partial, and as suggested alternative for _.extend (2.0) to support inherited properties.

@jamiebuilds
Copy link
Contributor Author

@jdalton I would definitely like to see a _.create method as it feels missing from this implementation. Is that necessary to move forward with this though? I don't see an open issue for _.create anywhere.

@jamiebuilds
Copy link
Contributor Author

Updated to take advantage of baseCreate.

if (_.isArray(obj)) {
accumulator = [];
} else if (_.isObject(obj)) {
var Ctor = obj.constructor;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conditional path really necessary? If the dev wants the return to be an instance of obj's class, why not leave it to them to pass in the proper accumulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it adds support for other native types as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning the typed arrays? If so, we might be able to do a good enough job by reordering the obj and accumulator conditionals:

if (obj == null) return accumulator != null ? accumulator : {};
if (accumulator == null) accumulator = obj.length === +obj.length ? [] : {};

It would generalize array-likes to an array, leaving it up the dev to be more specific if they want it.

I'm not particularly against this, I just think grabbing .constructor.prototype is ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypedArrays, Map, Set, etc., including anything created in user-land as well. Inferring the type is one of the things that makes transform so nice.

_.transform(new Backbone.Model, function() { /* ... */ }) instanceof Backbone.Model // >> true

I agree that accessing .constructor.prototype isn't very nice, but only if the user is doing something gross with it in the first place, in which case you can still specify the accumulator. Lodash's implementation works this way so @jdalton can speak to any problems that have come up because of it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only issue I can see with that method is constructor overrides, but thats cool by me

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: Your use cases for it would be valuable.

Underscore can't iterate a Map or a Set, so it still seems like the only gain over my above comment is instanceof? I'm 50-50 for supporting it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create use of _.transform came before the actual _.create method for me. It's a nice to have. The rationale for having it is you're transforming a value so the result should be of the same constructor. I've taken this approach in our clone method too preferring clones to be of the realm their original values. For me the biggest win is the ability to exit early and not providing a default accumulator so the create can be seen as a non-critical requirement that could be added at a later time.

@megawac
Copy link
Collaborator

megawac commented Dec 11, 2014

Looks good to go and I'm excited for this addition, ping @jashkenas

@megawac
Copy link
Collaborator

megawac commented Dec 28, 2014

I'm merging this if noone objects

@jasonLaster
Copy link
Contributor

Excited to see this land.

megawac added a commit that referenced this pull request Dec 29, 2014
@megawac megawac merged commit 9924dba into jashkenas:master Dec 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants