Remove d3 map #102
Remove d3 map #102
Conversation
sirahd
commented
May 17, 2017
sirahd
commented
- remove d3 map
- refactor a little more
- create a copy of options before supplement it, thus preventing causing side-effect to the original object
|
@domoritz please review this! |
src/parseOption.ts
Outdated
|
|
||
| itemData.each(function (value: string, field: string) { | ||
| // prepare title | ||
| for (let field of Object.keys(itemData)) { |
There was a problem hiding this comment.
You need a hasownproperty here
There was a problem hiding this comment.
I think Object.keys() already returns the properties that's not under prototype chain. So I'm not sure if we need an if to check that again
There was a problem hiding this comment.
But definitely use cosnt here.
There was a problem hiding this comment.
for let field of Object.keys() looks like an overkill. (It will run the loop twice.)
I think better use for..in and hasOwnProperty.
There was a problem hiding this comment.
I think the performance is almost the same. At least looking at jsperf. And for in with hasownproperty is not shorter. I'm okay with leaving this as is.
There was a problem hiding this comment.
Yeah but this is pretty nonstandard code. While hasOwnProperty is
There was a problem hiding this comment.
I've seen all of them but sure, let's go with for in as it is the most standard. Here is the jsperf btw: https://jsperf.com/object-keys-vs-for-in-with-closure/13
sirahd
force-pushed
the
master
branch
2 times, most recently
from
a6fea43
to
b0a377a
Compare
|
@domoritz Please ignore the first 4 commits. It was because I rebase onto master. |
|
Is this done? |
|
Yes. You can merge it. |
|
🚀 PR was released in |
