add blobstore to webapp by chuckluv · Pull Request #5 · chuckluv/Step

chuckluv · 2020-06-09T18:48:50Z

No description provided.

bsidhom · 2020-06-09T23:29:53Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-        System.out.println("Could not convert to int: " + maxString);
-        max = 1;
+        System.out.println("Could not convert to int|" + maxString+"|");
+        maxNumberOfComments = 1;


You might want to return an error code to the caller (probably SC_BAD_REQUEST).

bsidhom · 2020-06-09T23:32:30Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 @WebServlet("/Comments")
 public class DataServlet extends HttpServlet {
-  private int max=5;
+  private int maxNumberOfComments=5;


Since maxNumberOfComments can be set concurrently by different requests, all access to this field should be synchronized. We can discuss how to do that offline.

I'm confused as to why this isn't a constant. Either it's configurable (possibly with a constant default) in which case it should be a local variable or it isn't and should be final. Seeing this random shared mutable value is a big code smell.

bsidhom · 2020-06-09T23:33:57Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java


    List<Post> comments = new ArrayList<>();
-    FetchOptions fetchOptions = FetchOptions.Builder.withLimit(max);
+    FetchOptions fetchOptions = FetchOptions.Builder.withLimit(maxNumberOfComments);


This read needs to be synchronized.

bsidhom · 2020-06-09T23:34:32Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

-        System.out.println("Could not convert to int: " + maxString);
-        max = 1;
+        System.out.println("Could not convert to int|" + maxString+"|");
+        maxNumberOfComments = 1;


This write should be synchronized.

bsidhom · 2020-06-09T23:34:48Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 @WebServlet("/Comments")
 public class DataServlet extends HttpServlet {
-  private int max=5;
+  private int maxNumberOfComments=5;


Nit: keep final values separate from non-final values to draw attention to the fact that they have different mutability. In this case, I would put maxNumberOfComments below all private static final fields and add a newline to separate it.

bsidhom · 2020-06-09T23:53:11Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+    System.out.println(imageUrl);
+     PrintWriter out = response.getWriter();
+    out.println("<p>Here's the image you uploaded:</p>");
+    out.println("<a href=\"" + imageUrl + "\">");


It's risky to manually escape values within raw HTML like this. I'll try to figure out if there's a safer way to render this (e.g., using JSPs).

We're using JSPs now? I'm ded.

bsidhom · 2020-06-09T23:56:10Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+  public void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {
+
+    // Get the URL of the image that the user uploaded to Blobstore.
+    String imageUrl = getUploadedFileUrl(request, "images");


Note that imageUrl may be null, which you should be able to handle. For example, you probably don't want to add an anchor tag with href="null". Do you want to set different HTTP response codes in this case?

bsidhom · 2020-06-09T23:57:31Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+
+    // User submitted form without selecting a file, so we can't get a URL. (dev server)
+    if (blobKeys == null || blobKeys.isEmpty()) {
+      return null;


If you're returning null values as part of the API, you should add the @Nullable annotation tag to the method to indicate this so callers (and code linters) can see this. Alternatively, Return a java.util.Optional<String>.

bsidhom · 2020-06-09T23:59:01Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+      return null;
+    }
+
+    // Our form only contains a single file input, so get the first index.


Good point. The form only contains a single file input, but what if the user hijacked the form or manually submitted a request to submit multiple files? We probably don't need to worry about that here, but we may want to consider that during upload.

bsidhom · 2020-06-10T00:01:51Z

portfolio/src/main/webapp/index.html


      <ul id="fetch-comment"></ul>

+      <form id="uploaded-image" method="POST" enctype="multipart/form-data" >


Do you need to set an action URL here? (I don't know the answer to this, but I suspect that by default, this just sends a post request to the index page URL.)

KoopaKing · 2020-06-11T04:18:39Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

 @WebServlet("/Comments")
 public class DataServlet extends HttpServlet {
-  private int max=5;
+  private int maxNumberOfComments=5;


I'm confused as to why this isn't a constant. Either it's configurable (possibly with a constant default) in which case it should be a local variable or it isn't and should be final. Seeing this random shared mutable value is a big code smell.

KoopaKing · 2020-06-11T04:18:56Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

    long timestamp = System.currentTimeMillis();
    String maxString = request.getParameter(MAX_COMMENTS_PARAMETER_NAME);
-    if (userComment != null) {
+    if (userComment != null ) {


where's my formatter meme?

KoopaKing · 2020-06-11T04:20:45Z

portfolio/src/main/java/com/google/sps/servlets/DataServlet.java

      } catch (NumberFormatException e) {
-        System.out.println("Could not convert to int: " + maxString);
-        max = 1;
+        System.out.println("Could not convert to int|" + maxString+"|");


Please use String.format instead of these string concat chains.

System.out.println(String.format("Could not convert to int: \"%s\".", maxString);

Moreover I'm going to cut you a bug to switch to a real logger.

KoopaKing · 2020-06-11T04:22:14Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+
+    //Will change when walkthrough is finished
+    System.out.println(imageUrl);
+     PrintWriter out = response.getWriter();


/formattermeme

KoopaKing · 2020-06-11T04:23:55Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+   /** Returns a URL that points to the uploaded file, or null if the user didn't upload a file. */
+  private String getUploadedFileUrl(HttpServletRequest request, String formInputElementName) {
+    BlobstoreService blobstoreService = BlobstoreServiceFactory.getBlobstoreService();
+    Map<String, List<BlobKey>> blobs = blobstoreService.getUploads(request);


What fresh hell is? This API takes an HttpServletRequest?

Also you don't need to actually do anything for this. Just commenting on how much I hate this API.

KoopaKing · 2020-06-11T04:24:30Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+  private String getUploadedFileUrl(HttpServletRequest request, String formInputElementName) {
+    BlobstoreService blobstoreService = BlobstoreServiceFactory.getBlobstoreService();
+    Map<String, List<BlobKey>> blobs = blobstoreService.getUploads(request);
+    List<BlobKey> blobKeys = blobs.get("images");


Make this a constant instead of inlining it if it's a keyword.

KoopaKing · 2020-06-11T04:25:59Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+    // Get the URL of the image that the user uploaded to Blobstore.
+    String imageUrl = getUploadedFileUrl(request, "images");
+
+    //Will change when walkthrough is finished


Did you tho?

KoopaKing · 2020-06-11T04:26:51Z

portfolio/src/main/java/com/google/sps/servlets/FileHandlerServlet.java

+    System.out.println(imageUrl);
+     PrintWriter out = response.getWriter();
+    out.println("<p>Here's the image you uploaded:</p>");
+    out.println("<a href=\"" + imageUrl + "\">");


We're using JSPs now? I'm ded.

KoopaKing · 2020-06-11T04:33:20Z

portfolio/src/main/webapp/index.html

  </head>
-  <body onload="getComments()">
-    <div id="content">
+  <body   onload="getComments()" >


Did you reverse format this file?

KoopaKing · 2020-06-11T04:34:38Z

portfolio/src/main/webapp/script.js

+      .then((imageUploadUrl) => {
+        const messageForm = document.getElementById('uploaded-image');
+        messageForm.action = imageUploadUrl;
+       // messageForm.classList.remove('hidden');


add blobstore to webapp

b3bdc5b

chuckluv assigned bsidhom and KoopaKing Jun 9, 2020

bsidhom requested changes Jun 10, 2020

View reviewed changes

KoopaKing requested changes Jun 11, 2020

View reviewed changes


		<ul id="fetch-comment"></ul>

		<form id="uploaded-image" method="POST" enctype="multipart/form-data" >

Conversation

chuckluv commented Jun 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants