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

Better implementation of setInterval #23

Open
gabeno opened this issue Oct 29, 2013 · 10 comments · May be fixed by #38
Open

Better implementation of setInterval #23

gabeno opened this issue Oct 29, 2013 · 10 comments · May be fixed by #38
Assignees

Comments

@gabeno
Copy link

gabeno commented Oct 29, 2013

The introduction to the function setInterval says: setInterval(func, n) is a way to run a function every n milliseconds yet during implementation n has to be computed like so:

// Draw twenty frames per second (20 * 50 = 1000 milliseconds)
setInterval(frame, 50);

This is confusing. For consistency we could have the function take in millisecond units or have an explanation for the conversion.

@theophani
Copy link
Member

Hey :). setInterval is a native method to JavaScript and it takes milliseconds as the second argument. The comment is attempting to explain that if you want to draw a frame 20 times in one second, then you need to refresh every 50 milliseconds.

Perhaps this explanatory comment would be more clear if it said:

// Draw twenty frames per second (1000 / 20 = 50 milliseconds per frame)
setInterval(frame, 50);

@gabeno
Copy link
Author

gabeno commented Oct 29, 2013

Hi @theophani, your explanation is better, thank you. The main idea is to have x number of frames drawn per second. Once x is chosen the time per frame is then calculated. :)

@gabeno gabeno closed this as completed Oct 29, 2013
@xMartin
Copy link
Member

xMartin commented Oct 29, 2013

So should we improve the material?

@gabeno
Copy link
Author

gabeno commented Oct 31, 2013

@xMartin I think we should improve the comments to be more concise and easy to understand for a beginner programmer. I propose this:

\\ Set number of frames to be drawn per second at 20 (n = 1000 / 20 = 50 milliseconds)

The simple arithmetic better explains how the n in setInterval(func, n) is arrived at.

@gabeno gabeno reopened this Oct 31, 2013
@gabeno gabeno closed this as completed Jun 21, 2017
@xMartin
Copy link
Member

xMartin commented Jul 9, 2017

Should still be improved, I guess.

@xMartin xMartin reopened this Jul 9, 2017
@tobmaster
Copy link
Collaborator

tobmaster commented Oct 4, 2017

I was thinking of expressing whats going on in source code with good named variables.
Besides making it less abstract, it also teaches good practices to make the code "speaking".

// We want to draw twenty frames per second 
var framesPerSecond = 20;

// 1 second equals 1000 milliseconds.
var frameInterval = 1000 / framesPerSecond;
  
// setInterval expects the interval in milliseconds
setInterval(frame, frameInterval);

@xMartin
Copy link
Member

xMartin commented Oct 4, 2017

Personally, I like your version a lot. I've experienced learners that found it much easier to understand code with lots of names, that were overwhelmed with magic numbers.

On the other hand, some people do think in a more mathematical way and like the condensed way, playing with the numbers in place.

I wonder if we can provide multiple versions of code. But that's probably going too far.

With that said, I still think your version would be an improvement for most learners ;) Maybe we can add to the comments that this is equal to "draw a frame every 50 milliseconds".

@tobmaster
Copy link
Collaborator

Yeah but "mathematical thinkers" will be able to handle the variable names too.

@xMartin
Copy link
Member

xMartin commented Oct 7, 2017

@tobmaster Could you prepare a Pull Request? Would be highly appreciated.

@tobmaster
Copy link
Collaborator

@xMartin sure

tobmaster added a commit to tobmaster/js-beginners-1 that referenced this issue Oct 18, 2017
@tobmaster tobmaster self-assigned this Oct 18, 2017
@tobmaster tobmaster linked a pull request Oct 18, 2017 that will close this issue
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 a pull request may close this issue.

4 participants