Nit: make partContent a StringBuilder to avoid unnecessary copying

Using String.format(...) now for building String

Make this method static if doesn't use parent object state

Please add unit test coverage of this method.

This method cannot be made static because it uses boundary string which is dependent on parent object state. Also, added Unit Test for this method.

You want to strip the final semicolon. Why's that?

Content-Disposition parameters are prefixed by semi-colon, so removing the trailing one is technically correct.

However, I'm a little puzzled as to this approach where we pretend Content-Disposition parameters are headers and then we lift them out of the headers collection to construct the Content-Disposition header value. The list of headers should just be a list of HTTP headers where the content-disposition value is pre-constructed. If we need a helper to build that value, then let's build a createContentDispositionValue() function.

• I have now changed the way in developer will use this addPart(Map<String, String> headers, byte[] content) and added Helper function String createContentHeaderValue(String contentValue, Map<String, String> contentDispParameter).
• contentDispParameter map will contain (key=parameterName, Value=parameterValue). eg. (key="name", value="name-val").
• createContentHeaderValue helper function will construct the string "contentValue;name="nameval";..." which will be then passed in headers map as (key=Content-Disposition, value=string from helper function), (key=Content-Type, value=string from helper).

We can't make assumptions about the encoding of HTML content. If we keep this addHtmlPart method, it needs to accept a byte array and not a string.

Changed addHtmlPart to accept bytes.

There needs to be a space after the colon in the Content-Disposition header.

Added space after the colon in the Content-Disposition header.

Please create a reusable method and see if details can be encapsulated in that method.

rename to testMultipartPostWithHeadersMap

Renamed unit test function to testMultipartPostWithHeadersMap.

if you are reworking this use a StringBuilder.

Used StringBuilder now for part header's value-parameter.

Though the if statements may be correct you get more clarity for reading (and stop double checking values) by nesting like so:

if (filename !=null) {
    if (name != null) {
        blah;
    } else {
        blah;
    }
} else if (name!=null) {
   blah;
} else {
   blah;
} 

Restructured the conditional statements.

Are you missing a space again after ContentType:? More occurrences below.

Corrected it with space.

This is useful but what I'd like to see is coverage of the createPartHeader method which has a few conditional branches. If you wrote those simple tests you would see the formatting problems yourself and they would not need review iterations. Because you didn't explicilty add a test for createPartHeader I'm going to run through what I would expect to see for testing of a method like it:

• make createPartHeader static (including whatever state was used in its parameters)
• give the method default visibility
• add the annotation @VisibleForTesting or add comment // VisibleForTesting
• write numerous small test methods that cover every branch and condition of createPartHeader only (15 minutes of work to knock up)
• Keep the unit tests low level, only testing createPartHeader

@darrelmiller I know we have discussed briefly how much unit testing to do but this is an obvious easy win and easy to write. I'm happy for testing paths not to be coverered but it should be deliberate and argued as to why it is not being done.

On closer inspection the current test method performs no useful assertions so gives us no coverage at all of the changes made!

• Wrote unit tests for createPartHeader.
• createPartHeader cannot be made static because it uses addBoundary function which uses Boundary string that can be different for different multiparts. Suppose, developer wants to use Multipart at multiple places in application at those places the boundary value will be different so it cannot be made static, in turn then addBoundary and createPartHeader cannot be made static because of usage of non-static boundary member.

try catch unnecessary, just add throws Exception to the method. At the moment when fails we wouldn't even get a stack trace to help us.

Changed it to throw Exception.

This test is only asserting that we aren't going to throw an Exception. The ultimate output of any of these requests and a call to post is that an HTTP POST is made to

• a url
• with specific request headers
• and a chunk of json

The url building process should be tested elsewhere but your work has changed what we see in the second and third aspects. I haven't used the TestBase but it should enable you to assert things about the headers and content being posted. Can you add more assertions here?

• Added more assertions now to check url and request header.
• Content of request cannot be accessed once inside the request because post method calls and carries forward till request is sent on wire. So, I am checking byte content outside the request for being as notNull.

indent

indented now. Although IDE was showing indented earlier.

you have already taken out request object in line 487, why making pageReq.buildRequest(options) call again?

changed it to use request.post(...)

I would just drop this method. There isn't any commonality between Content-Type and Content-Disposition values. We can build helper methods for creating header values that are semi-colon delimited parameter lists at a later point that can be used in other places. This method just creates confusion and we are not using it except for in tests.

The content header's value and parameter string building is common in Content-Type and Content-Disposition. eg. Content-Disposition:form-data; name="someName"
Content-Type: text/html; otherparams=paramValue
Also, if some other headers is being provided apart from these then also same helper function can be used.