Skip to content

Conversation

@ihsan314
Copy link
Owner

@ihsan314 ihsan314 commented Jul 11, 2020

This pull request uses local memory to store comments, and allows them to be fetched with a GET request. The GET request returns the comment pairs in JSON format, and the webpage displays the comments upon page load.

Comment on lines 27 to 31
<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
<version>2.8.6</version>
</dependency>
Copy link

Choose a reason for hiding this comment

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

nit - let's keep dependencies alphabetical by groupId and then artifactId

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 will do.

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.util.ArrayList;
Copy link

Choose a reason for hiding this comment

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

Can you run this file through the gist? Should fix these imports for us.

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 sure.

import java.util.ArrayList;
import com.google.gson.Gson;

/** Servlet that returns some example content. TODO: modify this file to handle comments data */
Copy link

Choose a reason for hiding this comment

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

nit - remove this TODO comment, now that the class is handling comment data.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will do.

@ihsan314 ihsan314 requested a review from ccondit July 11, 2020 01:36
/** Servlet that returns some example content. */
@WebServlet("/data")
public class DataServlet extends HttpServlet {
List<String> messages = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this variable definition into the doGet method. If you define it at the class level, then every time someone refreshes your page, your code will add another 3 comments to the array. Eventually your server will OOM after so many visits.

Copy link

Choose a reason for hiding this comment

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

Did this change get made?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems doGet always returns the same messages, this could be made a constant ImmutableList instead.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh oops I don't think it got changed, I'll quickly move the variable definition.

/** Servlet that returns some example content. */
@WebServlet("/data")
public class DataServlet extends HttpServlet {
List<String> messages = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Did this change get made?

@ihsan314 ihsan314 requested a review from ccondit July 15, 2020 17:10
Copy link

@ccondit ccondit left a comment

Choose a reason for hiding this comment

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

Cool! LGTM.

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.

6 participants