Implemented the storage of comments as username-comment pairs #8
Conversation
ihsan314
requested review from
AKdroid,
ccondit,
fabielzuniga,
kaveh096 and
zonghengma
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| // import java.util.ArrayList; |
There was a problem hiding this comment.
nit - can you remove all the commented out code? let's not check in code that unused.
| @WebServlet("/data") | ||
| public class DataServlet extends HttpServlet { | ||
| ArrayList<String> messages = new ArrayList<>(); | ||
| HashMap<String, String> messages = new HashMap<String, String>(); |
There was a problem hiding this comment.
- Per my comment on the parent, can you use the interface here instead of the concrete type (Map instead of HashMap)?
- Java can infer the types from the declaration.
- Reduce visibility of member variables as much as possible.
| HashMap<String, String> messages = new HashMap<String, String>(); | |
| private final Map<String, String> messages = new HashMap<>(); |
There was a problem hiding this comment.
Ok, I will do this as well.
| public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
| String username = request.getParameter("username"); | ||
| String message = request.getParameter("comment-or-question"); | ||
| messages.put(username, message); |
There was a problem hiding this comment.
no need to take action now, but what if a client sends in a null username? or a null comment?
There was a problem hiding this comment.
I could disable null usernames/comments later otherwise likely no exceptions will be thrown if I do not disable null usernames/comments.
portfolio/src/main/webapp/script.js
Outdated
| messageContainer.innerText += message + '\n'; | ||
| for (const username in messages) { | ||
| messageContainer.innerText += username + ': ' + messages[username] + '\n\n'; | ||
| // messageContainer.innerText += username + '\n'; |
There was a problem hiding this comment.
looks like the commented out code is still here...
|
Also - a reminder to update your base branch when chaining PRs: |
portfolio/src/main/java/com/google/sps/servlets/DataServlet.java
Outdated
Show resolved
Hide resolved
portfolio/src/main/java/com/google/sps/servlets/DataServlet.java
Outdated
Show resolved
Hide resolved
| messages.add("It is great for storing data like this"); | ||
|
|
||
| response.setContentType("application/json;"); | ||
| String json = new Gson().toJson(messages); |
There was a problem hiding this comment.
No change needed, just FYI:
Google protocol buffers is a much better alternative to XML and JSON.
There was a problem hiding this comment.
Ok that's useful information for later web development, I will keep this in mind.
| for (const message of messages) { | ||
| messageContainer.innerText += message + '\n'; | ||
| for (const username in messages) { | ||
| messageContainer.innerText += username + ': ' + messages[username] + '\n\n'; |
There was a problem hiding this comment.
Do the \n\n have the desire effect? Since this is HTML?
There was a problem hiding this comment.
I believe it did, but I'll render again and check.
portfolio/src/main/java/com/google/sps/servlets/DataServlet.java
Outdated
Show resolved
Hide resolved
ccondit
left a comment
ccondit
left a comment
There was a problem hiding this comment.
This LGTM one that comment is removed...
portfolio/src/main/webapp/script.js
Outdated
| messageContainer.innerText += message + '\n'; | ||
| for (const username in messages) { | ||
| messageContainer.innerText += username + ': ' + messages[username] + '\n\n'; | ||
| // messageContainer.innerText += username + '\n'; |
There was a problem hiding this comment.
looks like the commented out code is still here...
This pull request builds on #7 by adding the usage of HashMaps and JavaScript dictionaries to store username-comment pairs. Further the pull request uses the POST method to update the list of username-comment pairs with textboxes placed inside a form.