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

finished homework 7 #2

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

finished homework 7 #2

wants to merge 1 commit into from

Conversation

davidjohnwoolf
Copy link

Not the fanciest style, however it is functional and I don't need any practice with css.

@@ -1,28 +1,55 @@
var MemoryGUI = (function () {
var MemoryGui = (function () {
Copy link

Choose a reason for hiding this comment

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

Your GUI is functioning pretty well. Code is laid out appropriately and all the thinking is being done by the game logic which is perfect.

I'd like to see the cards have a distinct back and front, if only to really simulate the idea that the game is actually flipping anything.

At the moment the game end up showing about three cards at once if a person clicks quickly enough through the cards. This is a small detail and seems nitpicky, but I think it's worth enhancing the functionality here so that only two cards are ever showing, either by tinkering with the setTimeout delay time, or maybe just disable clicking while two cards are showing.

The last suggestion I have is to consider adding some comments in your code. Your functions from lines 8 through 34 are pretty obvious in what they do, but around line 36 the meaning behind your variables and lines become less clear. Of course, we understand its ultimate function because it's the homework, but getting into the practice of leaving comments will help out later.

Good work overall!

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