Skip to content

Further change Acey Ducey JavaScript #337

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

Merged
merged 2 commits into from
Jan 2, 2022

Conversation

LukasMurdock
Copy link
Contributor

#332 implemented many of the same things I was thinking of implementing for Acey_Ducey here.

Notable further changes from 332:

  • Removed modules—brought utility functions back into aceyducey.js to support local HTML without web server—reference in issue 117
  • Moved back into a main()
  • Added support for Node.js by checking for window object in print() and input()
  • Added newGameCards() to:
    1. Generate three new random cards
    2. Handle the loop that ensures cardOne is less than or equal to cardTwo
    3. Use ES6 array destructuring assignment to return and assign cards
      • let [cardOne, cardTwo, cardThree] = getGameCards();
  • Added Prettier configuration file for code formatting
  • Added some comments
  • Created utility functions to easily validate yes//no input strings (This does feel like an abstraction taken a tad too far—or just the verbosity of the naming makes it seem more complex than it is)
  • Removed printCard() and added getCardValue() than can be passed into the regular print function
    • Added Nullish Coalescing Operator (??) to handle the undefined face value table lookup

@coding-horror coding-horror merged commit 8d06a43 into coding-horror:main Jan 2, 2022
@domenic
Copy link
Contributor

domenic commented Jan 2, 2022

It's pretty discouraging to see some of the changes I made undone in this manner. I won't be contributing to this repo anymore.

@coding-horror
Copy link
Owner

coding-horror commented Jan 2, 2022

Ah, sorry about that @domenic perhaps there can be two implementations in the same folder, if there is disagreement about the approach? @LukasMurdock perhaps you can work this out so we can move forward collaboratively?

@LukasMurdock
Copy link
Contributor Author

@domenic I apologize for undoing your changes in this manner. I don’t consider myself a pronounced programmer, and I was quite warmed to see that I thought of the same solutions for parts of acey_duey—that was genuinely fun and validating to see.

I never intended any of these changes to be prescriptive—I just wanted to possibly partake in moving the project forward. I agree, the coding changes here did go far in a (this-is-how-to-do-it) manner—not any of which I am set on, yet overwrote your changes in merge:

  • Modules or scripts?
  • Should we use Prettier? Is so, what configuration?
  • What platforms should we support? Should we support Node.js?
  • Are the “utility” functions I wrote even worthwhile?
  • Let or const?

Some of these questions surfaced in the JavaScript rendering target issue, and I would love to have your feedback on how we can better move forward here—as I greatly appreciate your contributions. I am new to all things here and I would further appreciate if you could show me a better way to contribute code before such prior questions are answered—without stepping on toes like what just happened here.

I do hope that you can still find fun in contributing to this repo.

@coding-horror
Copy link
Owner

Maybe we have a dedicated topic for discussing the JS framework / approaches to use for each program?

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.

3 participants