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

fix stock id in 'randomStockUpdate' #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ItaHeld90
Copy link

@ItaHeld90 ItaHeld90 commented May 1, 2018

stockId was mistakenly written as stockIdx

@getify
Copy link
Owner

getify commented May 1, 2018

The one-letter typo is indeed a mistake, but I don't understand why you're making the other changes?

@ItaHeld90
Copy link
Author

I figured it would be less error prone and more expressive to make the generated random id into a variable ( instead of repeating 'stockIds[stockIdx]' through the function ).
not too critical, especially considering the code is for demonstration purposes.
If you prefer, I'll reduce the PR to fix only the typo.

@ItaHeld90
Copy link
Author

I know it's probably not the right forum for this, but anyway...
I really enjoyed your book, it really made me boost up my code structure and way of thinking.
Just wanted to thank you for all the hard work and contribution for the community :)

@getify
Copy link
Owner

getify commented May 2, 2018

For now I prefer to only fix typos, not make substantive changes. I would defer those changes for a second edition.

A reduced PR for this typo (with potentially a separate issur/PR for the other suggestions) would be appreciated. Thanks!

@ItaHeld90 ItaHeld90 closed this May 2, 2018
@ItaHeld90 ItaHeld90 reopened this May 2, 2018
@ItaHeld90
Copy link
Author

ItaHeld90 commented May 2, 2018

Opened a new PR #160 with just the typo fix.

I'm not sure what to do with this PR, I guess you can close it, or leave it for future reference/suggestion as you offered.
Thanks again :)

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.

2 participants