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 apply #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add apply #23

wants to merge 2 commits into from

Conversation

tamirisapbonicenha
Copy link

@tamirisapbonicenha tamirisapbonicenha commented Oct 8, 2019

Add my own Function.prototype.apply().
#21

while (otherThis.hasOwnProperty(uniqueID)) {
uniqueID = "00" + Math.random();
}
otherThis[uniqueID] = this;
Copy link
Member

Choose a reason for hiding this comment

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

Can you use this line of code?
otherThis.__proto__.uniqueID = this;

Choose a reason for hiding this comment

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

@TheSTL Why do we need to use this instead of otherThis[uniqueID] = this; . Can you elaborate ?

Copy link
Member

@TheSTL TheSTL Oct 12, 2019

Choose a reason for hiding this comment

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

By doing this we are making a pair of key-value in otherThis where key is some random id and value is function on which apply is called. When we call the function of otherThis (otherThis.uniqueID(...arr)) then in function this value will be otherThis.
@ashishdutta007

Copy link

@ashishdutta007 ashishdutta007 Oct 13, 2019

Choose a reason for hiding this comment

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

@TheSTL The same is being done by @tamirisapbonicenha in line no 30 & 35
result = otherThis[uniqueID]();.
The only benefit what I can see from using otherThis.__proto__.uniqueID = this; in place of
otherThis[uniqueID] = this; is that there is no chance of collision of the unique id in the prototype object.
Moreover is __proto__ is a browser implementation , instead of a built in js implementation.
Is there something I'm missing ?

Copy link
Member

@TheSTL TheSTL Oct 13, 2019

Choose a reason for hiding this comment

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

Actually problem is the uniqueId property is printed .You can check snapshot #23 (review)
@ashishdutta007

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @ashishdutta007 for telling me Moreover is __proto__ is a browser implementation , instead of a built in js implementation.. I'm testing things on browser .

Comment on lines +32 to +35
for (let i = 1, len = arr.length; i < len; i++) {
args.push("arr[" + i + "]");
}
result = eval("otherThis[uniqueID](" + args + ")");
Copy link
Member

@TheSTL TheSTL Oct 8, 2019

Choose a reason for hiding this comment

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

And for 32-35 can you use this line of code?
result = otherThis.uniqueID(...arr);

result = eval("otherThis[uniqueID](" + args + ")");
}

delete otherThis[uniqueID];
Copy link
Member

Choose a reason for hiding this comment

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

delete otherThis.__proto__.uniqueID

Copy link
Member

@TheSTL TheSTL left a comment

Choose a reason for hiding this comment

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

Hey @tamirisapbonicenha , your implementation is great 👍
Can you use let instead of var?
But there are few cases where your code doesn't match the apply method .

uniqueId property is printed

image
To handle this bug i have commented where changes are required .

Should throw error of arr argument is not array

image
Can you handle this bug?

@tamirisapbonicenha
Copy link
Author

Hi @TheSTL sorry for the delay, trouble made me disappear these days. I read the conversations and I will work on them on Sunday, all right?

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.

4 participants