yichuandoris / MI-449-SS17-740-js-conditionals-uMDt4P

0 stars 0 forks source link

Project Feedback #1

Open yichuandoris opened 7 years ago

yichuandoris commented 7 years ago

@egillespie Can you take a look at this? It's hosted here and meets the following criteria:

Hi, this is my first version after several tries and I hope it looks not too far from the requirements. Kindly advise, thank you! - Doris

egillespie commented 7 years ago

Hi @yichuandoris, I really like your adventure, it's very creative and I like the way you use two buttons to get the full experience. :airplane: :handbag:

This is a fantastic start. I have a few small suggestions:

Missing confirm

I didn't see a call to confirm in your JavaScript. >_< Do you mind adding one in to satisfy the first criteria?

Normalize strings immediately after they are collected

I see that you're properly normalizing your strings (ready, greeting, destination), but the code is after your if statements. That means if I type "France" instead of "france", I will get the "Cooooool" message instead of the "Bingo" message.

If you move your string normalizing code above the if statements, I should be able to type uppercase letters and see the expected message.

Use parseInt to convert hour to a number

After you collect hour in your code, can you convert it to a number using parseInt? I see you make sure that hour is set to 1 if a non-number is typed, but if I type the string "3" the conversion isn't happening (and we want to make sure you know how to do that). :smile:


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! :rocket:

yichuandoris commented 7 years ago

Hi Erik, Thank you. I just updated the project.
1) I've added some "confirm" in the first part of the game - sorry I guess I removed it when I was revising my project. But I don't really see the difference between the "confirm" and "prompt" method. 2) I relocated those normalized strings while I am not sure if I locate them right - so just moved them right before the "if" statement they belong to respectively. 3) I added the parseInt to convert "hour" to a number. Same question here: I'm not sure if it is placed at the right spot - so I added it before the "var hour." 4) I found that I didn't prevent users from quit in the first question(greeting), so I added some lines (line 7 and 8) in cases of null and undefined. - I also have a question here: do we have to add the lines (to warn a "cancel" action done by users) to every question we asked in the story?

I also tried several times checking everything acts like what I expect, hope I didn't miss a thing. Kindly let me know if I made them right. Thank you. - Doris

egillespie commented 7 years ago

Hi @yichuandoris, thanks for the followup! :smiley_cat: Let me address your points one at a time, some of them will result in some more code changes:

1. Difference between prompt and confirm

Both prompt and confirm are used to collect input from the user. prompt has a text box in it that lets a user type whatever they want and a string or null is returned. So prompt is for getting a string from a user.

confirm has two buttons: "OK" and "Cancel". If a user clicks "OK" then confirm will return true, if "Cancel" is clicked then false will be returned. So confirm is for getting a true/false value from a user.

Here's one way to use confirm:

var proceed = window.confirm('Are you sure you want to continue?')
if (proceed) {
  window.alert('Okay, let\'s go!')
} else {
  window.alert('Sorry to hear that. Goodbye.')
}

Would you mind changing one of your uses of confirm to get the value from the user and do something with it like the example above? All of your other calls to confirm can be changed to alert.

2. Normalizing values

Normalizing a value should happen after you get the value from a user. Let's consider these three lines of your code:

greeting = greeting || ''
greeting = greeting.trim()
var greeting = window.prompt('Welcome to the crazy where-to-go world - how are you today?')

Since a user does not provide a value for greeting until the third line, the first two lines will not have any effect because line one sets greeting to '' and the second line trims the greeting, which means greeting will still be ''. Then on line 3, if a user types ' Hello', that's what will be saved in greeting forever.

Instead, you should get the value from the user first, then normalize the value:

var greeting = window.prompt('Welcome to the crazy where-to-go world - how are you today?')
greeting = greeting || ''
greeting = greeting.trim()

If a user types ' Hello', you will make sure greeting is at least an empty string on line 2, then trim the spaces from the front and back on line 3, leaving greeting set to 'Hello'.

It looks like there are X places where you need to normalize values after you collect them from the user: greeting, hour, destination, country, and state. Do you mind updating those?

3. Where to use parseInt

You're pretty close! You just need to move the call to parseInt down one line as suggested in #2 above.

4. Where to add a cancel message

I think the way you're showing messages is great! :ok_hand: In this game it definitely made the game more interactive, and that's something players enjoy!


If you have other questions about this, please message me on Slack (erik.gillespie). If you think a meeting would help you better, I'm more than happy to do that too!

After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! :rocket:

yichuandoris commented 7 years ago

Hi, thanks Erik @egillespie ! I just pushed the update code again. Would you mind take another look at it? Thank you! Doris

egillespie commented 7 years ago

This is coming along really well! :dizzy: Something I didn't notice the first time looking at the code is now making parts of your adventure not work.

Could you replace all of the toUpperCase calls with toLowerCase? For country and state, if I type in capital letters, I don't get the correct results because you're looking for the value "michigan" (for example). Converting the inputs to lowercase instead of uppercase should fix this right up!

Thanks! :smile:

yichuandoris commented 7 years ago

Wow, how silly I am. I was thinking to make them lowercase but automatically typing uppercase! XD Thanks for reminding me, and I guess I also forgot to clean my cached data so I didn't find this bug. They should be good, now. Both "France" and "france" get a Bingo now.

egillespie commented 7 years ago

Awesome! Thanks for all of the hard work on this project, it's really great! :shipit: