Conversation
gradywardgoogle
left a comment
gradywardgoogle
left a comment
There was a problem hiding this comment.
Josiah, great work. This code is exceptionally clear, well formatted, consistent and readable. I've rarely seen this level of attention to detail from interns, and I really appreciate how easy it makes my review process! Keep up the good work.
| /** Servlet that returns some example content. TODO: modify this file to handle comments data */ | ||
| @WebServlet("/data") | ||
| public class DataServlet extends HttpServlet { | ||
| ArrayList<String> employeeComment = new ArrayList<String>(); |
There was a problem hiding this comment.
Can we refactor this to get it to the point where we don't store state in the servlet? Ideally every reuqest we make to the servlet will be independent from any other request - and state in the servlet seems to counter-act that.
I think this could just be a method variable in doGet, right? then you could pass it into getNumComments.
| try { | ||
| requestedNum = Integer.parseInt(userNum); | ||
| } catch (NumberFormatException e) { | ||
| System.err.println("Could not convert to int: " + userNum); |
There was a problem hiding this comment.
I definitely appreciate the error handling! But instead of using print statments, let's throw an exception (perhaps an IllegalArgumentException here). In general, we want to code defensively, which means we want to fail requests that are poorly formed, or don't conform to our expectations. Throwing an exception is a good way to indicate that to the calling user (whether that's Javscript that you write, or an accessor of your API).
There was a problem hiding this comment.
Can you do that throughout? IllegalArgumentExceptions are a good semantic choice in many cases (where you're validating the input of a user), but so are IllegalStateExceptions (when you're validating an assumption you have about what a function or datastore will contain/return)
|
|
||
| //Limit the number of comments | ||
| for(int i = 0; i < getNumComments(request); i++) { | ||
| response.setContentType("text/html;"); |
There was a problem hiding this comment.
I think this probably shouldn't go in the for loop? We probably only need to set it once.
| <p>My name is Josiah Caldwell, I am a computer science student at | ||
| <div id="section"> | ||
|
|
||
| <p><img src="/images/Mark.png" alt="Mark of the Outsider" id="Mark">My name is Josiah Caldwell, I am a computer science student at |
There was a problem hiding this comment.
meganit: we'll want to use lower-dash-case for HTML classes and IDs. This isn't a huge deal, but it will be helpful as we build up a bigger code base.
2 participants
Complete Week 3
Add functional comments
Change color scheme