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

Comments with nicknames #7

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

beingPurple
Copy link
Owner

allows users to comment with changeable nicknames

Copy link

@akochar-sps akochar-sps left a comment

Choose a reason for hiding this comment

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

Overall looks okay, left some improvement comments.



}

response.setContentType("application/json;");
String json = new Gson().toJson(tasks);
System.out.println("get: " + tasks);
// System.out.println("get: " + tasks);

Choose a reason for hiding this comment

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

Dont add code as comments. Please remove.

//add in nickname to commstream
UserService userService = UserServiceFactory.getUserService();
String nickname = getUserNickname(userService.getCurrentUser().getUserId());
System.out.println("nickname: " + nickname);

Choose a reason for hiding this comment

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

Is there a reason you are printing this. Debug comments like this can result in privacy concerns.

UserService userService = UserServiceFactory.getUserService();
String nickname = getUserNickname(userService.getCurrentUser().getUserId());
System.out.println("nickname: " + nickname);
// commStream.add(nickname + ": ") //a way to make nicknames permanent

Choose a reason for hiding this comment

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

Again remove commented code in all places.

Comment on lines 78 to 83
if (entity == null) {
return null;
}
String nickname = (String) entity.getProperty("nickname");
return nickname;
}

Choose a reason for hiding this comment

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

You could use a try and catch block here, instead of checking for entity null and catch on NullPointerException.

Comment on lines 43 to 48
out.println("<p>Set your nickname here:</p>");
out.println("<form method=\"POST\" action=\"/nickname\">");
out.println("<input name=\"nickname\" value=\"" + nickname + "\" />");
out.println("<br/>");
out.println("<button>Submit</button>");
out.println("</form>");

Choose a reason for hiding this comment

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

you can just do
out.println("

Set your nickname here:

" +
"<form method="POST" action="/nickname">" +
"<input name="nickname" value="" + nickname + "" />" +
"
"+
"Submit" +
"");

Choose a reason for hiding this comment

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

Better yet you can move the comment to a nickname_template.html file and just read it from there. This way you separate html from java. Here and other places.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok! I have put in the code in the first comment and will put it into another file in the next branch.

// The put() function automatically inserts new data or updates existing data based on ID
datastore.put(entity);

response.sendRedirect("/home");

Choose a reason for hiding this comment

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

Can you comment why you want to redirect user to home here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No particular reason other than that it was included in the week 3 user-nicknames example. Thank you for catching it! It will be deleted by the next commit

Comment on lines 86 to 90
if (entity == null) {
return "";
}
String nickname = (String) entity.getProperty("nickname");
return nickname;

Choose a reason for hiding this comment

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

try {
return (String) entity.getProperty("nickname");
} catch ( NullPointerException e) {
// TODO: Log exception in server logs for records.
return "";
}

// console.log(text);
parsed = JSON.parse(text);
parsed.reverse();
// console.log(parsed);

Choose a reason for hiding this comment

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

Remove debug code here and other place.

document.getElementById("text-field").style.display = "none";
}
else {
fetch('/data').then(response => response.text())//fetch from data

Choose a reason for hiding this comment

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

What if fetch fails can you add error case here and other places.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done! I have made a function that throws an error if fetch fails and have implemented in every fetch call in this js file. It will appear in the next commit

Comment on lines 82 to 86
if (entity == null) {
return "";
}
String nickname = (String) entity.getProperty("nickname");
return nickname;

Choose a reason for hiding this comment

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

try {
return (String) entity.getProperty("nickname");
} catch ( NullPointerException e) {
// TODO: Log exception in server logs for records.
return "";
}

Choose a reason for hiding this comment

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

I see this logic being used in multiple files, maybe pull it out to its own helper or util class, so you don't have to repeat code.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok! I have implemented the code in the first comment(will appear in the next commit) and will pull out the logic in the next branch I make. Thank you!

beingPurple added 2 commits June 28, 2020 14:45
…ause the problems are addressed here; it is not a new venture or addition per se
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.

2 participants