Skip to content

Issue35#58

Merged
kappsegla merged 19 commits into
mainfrom
issue35
Feb 11, 2022
Merged

Issue35#58
kappsegla merged 19 commits into
mainfrom
issue35

Conversation

@FelixJacobsen

@FelixJacobsen FelixJacobsen commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

This request should resolve #35

My thought was to make the objects reusable to save heap memory. Therefore, all fields are reset after use.

The the implementation of HttpServlet is to be made in other issues to keep down the code volume for one pull-request.

@GreenGard GreenGard requested review from GreenGard and removed request for GreenGard February 3, 2022 09:55
@Vimbayinashe

Copy link
Copy Markdown
Contributor

I can have a look at this pull request later on in the day as I first to need to do some reading in order to understand HttpServlet better.

@Vimbayinashe Vimbayinashe self-requested a review February 3, 2022 12:42
@kappsegla kappsegla mentioned this pull request Feb 3, 2022
@robinkorkmaz1 robinkorkmaz1 self-requested a review February 4, 2022 02:04
Comment thread src/main/java/org/fungover/storm/httphandler/Request.java Outdated
Comment thread src/main/java/org/fungover/storm/httphandler/Request.java Outdated
Comment thread src/main/java/org/fungover/storm/httphandler/Response.java
Co-authored-by: Vimbayinashe Mandaza <55695513+Vimbayinashe@users.noreply.github.com>
Comment thread src/main/java/org/fungover/storm/httphandler/Response.java Outdated
Co-authored-by: Vimbayinashe Mandaza <55695513+Vimbayinashe@users.noreply.github.com>
@Vimbayinashe

Copy link
Copy Markdown
Contributor

Overall this code looks great! I have added a few suggestions in the code.

The constructor with parameters for Response class is not included in the test coverage and perhaps it may be used in one of the tests to give the class 100% code coverage tests.

@Vimbayinashe

Copy link
Copy Markdown
Contributor

I have one more suggestion 🙈 . It covers different files so I will copy a diff log here. It would be safer if response.getHeaders() never returns a null value as it is currently able to do in the tests.

/src/main/java/org/fungover/storm/httphandler/Response.java
public class Response {
   
 //initialize this.headers before calling this.reset()
     public Response() {    
         this.headers = new HashMap<>();
         this.reset();
     }
 
// use headers.clear() instead of assigning null
     public void reset() {
         this.statusCode = 0;
         this.body = new byte[0];
          this.headers.clear();
     }
 
//assert that result is equal to empty map instead of null
 src/test/java/org/fungover/storm/httphandler/ResponseTest.java b/src/test/java/org/fungover/storm/httphandler public class ResponseTest {
    @Test
   void resettingResponseShouldSetHeadersToNull(){
        Map<String,String> map = new HashMap<>();
        map.put("1","3");
        response.setHeaders(map);
        response.reset();
         var result = response.getHeaders();
         assertThat(result).isEqualTo(Map.of());
     }

@robinkorkmaz1 robinkorkmaz1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks good!

@helenahalldiniths

helenahalldiniths commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

As far as I know, this looks like a good solution. I saw that Robin is assigned as a reviewer and left an "approving" comment but did not approve the code. Since I am not educated enough on this subject I will wait for Robin to make an approval. If not, I can look in to it later this afternoon.

@kappsegla kappsegla self-requested a review February 11, 2022 13:16
@kappsegla kappsegla linked an issue Feb 11, 2022 that may be closed by this pull request
@kappsegla kappsegla merged commit 1341c22 into main Feb 11, 2022
@kappsegla kappsegla deleted the issue35 branch February 11, 2022 13:20
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.

Design interfaces and classes for handling Httprequest

5 participants