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

Refactor Tock to an OO implementation #21

Closed
wants to merge 1 commit into from

Conversation

ydaniv
Copy link
Contributor

@ydaniv ydaniv commented Dec 25, 2014

This refactor fixes misuse of this inside _tick() which contaminated global namespace.

This implementation is much more memory efficient since it's not redefining the inner functions of Tock on every instance generation. Instead they reuse all the module's private functions via closure and Tock's public methods on its prototype.

Note that everything should remain the same, except for passing reference to a method of a Tock's instance. This should be done with care since state is now managed in an OO way via this.

Fixed misuse of this inside _tick() which contaminated global namespace
@ydaniv
Copy link
Contributor Author

ydaniv commented Jan 5, 2015

@mrchimp should I resolve these conflicts? Are you even considering this PR?

BTW, I've found bugs in the tick mechanism and fixed it for the regular Tock, i.e. not countdown.
If this PR is of any interest I can also fix it for countdown mode and send updates on this PR.

@mrchimp
Copy link
Owner

mrchimp commented Jan 5, 2015

Hi Yehonatan

Sorry, been busy over the holidays. Back to work today. Definitely
considering merging this. If you could resolve the conflicts and update the
PR that would be ideal.

Cheers!

On 5 January 2015 at 08:35, Yehonatan Daniv notifications@github.com
wrote:

@mrchimp https://github.com/mrchimp should I resolve these conflicts?
Are you even considering this PR?

BTW, I've found bugs in the tick mechanism and fixed it for the regular
Tock, i.e. not countdown.
If this PR is of any interest I can also fix it for countdown mode and
send updates on this PR.


Reply to this email directly or view it on GitHub
#21 (comment).

@ydaniv
Copy link
Contributor Author

ydaniv commented Jan 5, 2015

Sure thing 👍

@ydaniv
Copy link
Contributor Author

ydaniv commented Jan 8, 2015

Closing, see resolved and updated PR #22

@ydaniv ydaniv closed this Jan 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants