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

Add enhancements to existing prototype features in the go feature. #22

Merged
merged 19 commits into from
Aug 7, 2020

Conversation

lilymzhou
Copy link
Collaborator

@lilymzhou lilymzhou commented Aug 3, 2020

  • Add style (style.css)
  • Hide hint and proceed buttons when they're unneeded, and show only once they're needed.
  • User can exit the hunt by clicking an exit button. A window pops up confirming that they want to exit.
  • Add additional messages to display when appropriate: a message appears to tell the user if their guess is correct or incorrect; a summary of all destinations found is displayed after the user finishes the hunt.

Start:
PR1_P1

After clicking the start button:
PR1_P2

After inputting the correct destination name:
PR1_P4

After completing the hunt:
PR1_P5

Copy link
Contributor

@megankj megankj left a comment

Choose a reason for hiding this comment

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

You might want to think about creating a new ScavengerHuntManager javascript class that is responsible the business logic of managing the flow of the scavenger hunt.

This would be convenient because it would allow you to better encapsulate functions and variables, which right now are global.

For example, something like this:

class ScavengerHuntManager {
constructor() {}
fetchData() {
this.scavengerHuntData = // result of the fetch call
this.currentIndex = // whatever
}
start() {}
proceed() {}
exit() {}
etc.
}

Don't feel like you need to do this with this PR, but I would encourage you to think about how you can use object oriented programming here.

src/main/java/com/google/sps/servlets/GoDataServlet.java Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
@lilymzhou lilymzhou requested a review from megankj August 5, 2020 20:33
Copy link
Collaborator

@tjgoog tjgoog left a comment

Choose a reason for hiding this comment

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

Haven't looked at all of it, but these are the comments I have so far

src/main/java/com/google/sps/servlets/GoDataServlet.java Outdated Show resolved Hide resolved
@@ -22,10 +22,12 @@
// For saved scavenger hunts: represents which destination the user is currently looking for.
private int index;
private String city;
private String guess;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of storing a guess as part of the ScavengerHunt data? Maybe there's an upcoming change that you're planning that would help me understand better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this in primarily for the convenience of being able to easily convert all of the data sent to /go-data into one JSON object -- it's not really relevant to the ScavengerHunt data itself.

src/main/java/com/google/sps/servlets/GoDataServlet.java Outdated Show resolved Hide resolved
} catch (Exception e) {
}

nameGuess = request.getParameter(NAME_PARAMETER);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, this design is that the client is expected to send only an index XOR a guess. Because those are two different pieces of functionality, they should be more clearly separated. I think separating them by having different URL paths for each would be the most simple and clear way of doing this*. I'm not sure how to configure a servlet to handle multiple different URL paths, but I imagine there's a way, otherwise you could write an additional servlet class for the different path. I can help you look for a way to do multiple paths in the same class later on if you'd like.

  • Other ideas include having an additional parameter that specifies which set of functionality the client wants from the server for this request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes sense -- I think it'd be clearer if I split GoDataServlet into two separate servlets to handle getting the scavenger hunt data and getting the user's answer separately.

@lilymzhou lilymzhou requested a review from tjgoog August 5, 2020 22:23
src/main/java/com/google/sps/servlets/Constants.java Outdated Show resolved Hide resolved
src/main/webapp/go.html Outdated Show resolved Hide resolved
src/main/webapp/go.css Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Outdated Show resolved Hide resolved
src/main/webapp/go.js Show resolved Hide resolved
src/main/webapp/go.js Show resolved Hide resolved
@lilymzhou lilymzhou requested a review from tjgoog August 7, 2020 00:22
Copy link
Collaborator

@tjgoog tjgoog left a comment

Choose a reason for hiding this comment

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

🎉

src/main/webapp/go.html Outdated Show resolved Hide resolved
@lilymzhou lilymzhou merged commit 7fa7a88 into master Aug 7, 2020
@lilymzhou lilymzhou deleted the PrototypeGo branch August 7, 2020 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants