FreakingGrant / MI-449-js-conditionals

0 stars 0 forks source link

Project Feedback #1

Open FreakingGrant opened 4 years ago

FreakingGrant commented 4 years ago

Write a short choose-your-own adventure game in JavaScript

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

egillespie commented 4 years ago

Oh wow, what a throwback adventure! I really like the functions you've introduced and the documentation, all good things! 🤘

You're hitting a lot of the requirements as well so you're off to a great start. Here are some ways I think your code can be improved. Can you try these out?

Use the result from a confirm popup

It looks like you are displaying a few confirm popups, but you don't use the results:

window.confirm('You are about to embark on a quest for the Wumpus.')
window.confirm('The wumpus is hidden in a room from ' + minRoom + ' to ' + maxRoom + '. You have been randomly dropped in a room between ' + minRoom + ' and ' + maxRoom + '.')
window.confirm('The wumpus moves about randomly. He may be in a room at some time, but then move to another room. You must keep moving to find the Wumpus.')
window.confirm('You can smell the Wumpus up to two rooms away.')
window.confirm('Your goal is find the Wumpus. It may be happy, or it may be ferral. You could live, you could die. Only one way to find out.')

This can be a little misleading to users because they will be presented with two buttons, but their choice doesn't have any effect on the game.

Would you mind replacing these confirm popups with alert popups and then add a confirm somewhere and use the result to do something in the game?

Use || to combine conditions instead of using else if

It looks like you have a few if/else if conditions where the code blocks are the same if more than one condition is true. For example:

  while (decision === null || decision === '') {
    decision = window.prompt('What will you do? Type:\n1 - To NOT kill the Wumpus.\n2 - Kill the Wumpus.\n3 - Kill yourself.')
  }
  while (isNaN(parseInt(decision))) {
    decision = window.prompt('What will you do? Type:\n1 - To NOT kill the Wumpus.\n2 - Kill the Wumpus.\n3 - Kill yourself.')
  }
  while (decision < 1 || decision > 3) {
    decision = window.prompt('What will you do? Type:\n1 - To NOT kill the Wumpus.\n2 - Kill the Wumpus.\n3 - Kill yourself.')
  }

In these cases, you can avoid code duplication and actually (but subtly) improve the performance of your code by using || to combine these conditions:

if (decision === null || decision === ''
  || isNaN(parseInt(decision))
  || decision < 1 || decision > 3) {
    decision = window.prompt('What will you do? Type:\n1 - To NOT kill the Wumpus.\n2 - Kill the Wumpus.\n3 - Kill yourself.')
}

Would you mind updating all of your if/else if conditions to use || where appropriate?


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! 🚀

FreakingGrant commented 4 years ago

Made all the changes!

egillespie commented 4 years ago

Your use of confirm is hilarious! 😂

I like your use of || in the one while loop. Would you mind applying the same technique to the loops starting on line 34 to avoid code duplication there as well?

FreakingGrant commented 4 years ago

Done and done, hopefully for the last time!

egillespie commented 4 years ago

Close! It looks like there's a missing } after your while loop now and the code is breaking. Would you mind adding that back?

FreakingGrant commented 4 years ago

Whoops, fixed it and tested it on GH-pages, should be good now!

egillespie commented 4 years ago

Aww yeah, looks and works great. Nicely done! 🤘 :shipit: