Use reduce in View.configureTriggers

[megawac]

Minor refactor. This is how I'd write this function. Feel free to reject

[jasonLaster]

👍

[samccone]

👍

[jamiebuilds]

Hmm, syntactically it could be debated if reduce is all that much better than each, but from a micro-perf perspective a reduce is slower than an each (http://jsperf.com/build-object-via-each-vs-reduce).

👎 Thanks though

[samccone]

@thejameskyle this reduced intermediate state in the method tho, so I am a fan

[jamiebuilds]

How exactly does it get rid of state?

[jamesplease]

In most situations, this method is called a single time – when a view is created – so I wouldn't consider speed to be a top priority for this method. I prefer this more compact version.

👍

[samccone]

var triggerEvents = {};

is internal state to the method that is unneeded.
reduce accomplishes the goal in a very functional approach

[jamiebuilds]

@jmeas Actually when uglified, the _.each loop is smaller.

// _.reduce: 185
function configureTriggers(){if(this.triggers){var r=this.normalizeUIKeys(_.result(this,"triggers"))
return _.reduce(r,function(r,i,e){return r[e]=this._buildViewTrigger(i),r},{},this)}}

// _.each: 176
function configureTriggers(){if(this.triggers){var i={},r=this.normalizeUIKeys(_.result(this,"triggers"))
return _.each(r,function(r,e){i[e]=this._buildViewTrigger(r)},this),i}}

@samccone using _.reduce doesn't make it any more functional or remove any real state for a user. So I don't see that as a good reason for this change.

[jamiebuilds]

Also, @megawac why not add a _.transform function to underscore like lodash, this is exactly what that function is for.

[megawac]

Yup, the two reasons @thejameskyle pointed our was why I mentioned I dig if you want to reject.

I would argue that this approach is more functional. Akin to how map can be rewritten using each or a good ol' for loop, this code I feel is clearer using reduce.

The performance issue observed with using reduce is relevant to a current oddball underscore issue that's been on the back burner for a while. I've tried my hand at it once or twice It should be fixed in a subsequent version

[megawac]

And I've been wanting to add _.transform for a while (it's my favourite single lodash function). If you want to make a PR/issue for it I'd give it a big +1
(I try not to open more than 10 prs at a time per project tho :p)

[samccone]

I would argue that this approach is more functional. Akin to how map can be rewritten using each or a good ol' for loop, this code I feel is clearer using reduce.

Yep this is the same rational I have

[jamiebuilds]

Opened a PR in underscore to add _.transform.

[jamiebuilds]

While it might be more "functional" (sorry, I have trouble using that word when describing underscore methods), the issue I take with this (besides what I've already mentioned about performance and file size) is that I don't think a reduce or fold method is semantically correct for what is happening here.

[jasonLaster]

Discussion happened off thread. The consensus was that this was a nice stylistic change.