Hi, Comments in line ...
2009/5/24 Greg Brown <[email protected]> > Chris, > > This looks great - thanks for jumping on it so quickly. I made a few minor > changes: > > Couple of questions: > > - Should we be using addRequestProperty() here when index > 0? > > for (String key : requestHeaders) { > for (int index = 0; index < requestHeaders.getLength(key); index++) > { > connection.setRequestProperty(key, requestHeaders.get(key, > index)); > } > } That's updating the actual connection, not the underlying properties classes. However, you might have spotted something else. The Javadoc for that set method is: "NOTE: HTTP requires all request properties which can legally have multiple instances with the same key to use a comma-seperated list syntax which enables multiple properties to be appended into a single property." Which probably explains why it was previously called RequestProperties, i.e. in line with the HttpURLConnection class ... doh! Personally, I still prefer "RequestHeaders" as it seems more explicit than RequestProperties. So, it should probably look like this: for (String key : requestHeaders) { StringBuffer buffer = new StringBuffer(); for (int index = 0; index < requestHeaders.getLength(key); index++) { if (index > 0) { buffer.append(","); } buffer.append(requestHeaders.get(key, index)); } connection.setRequestProperty(key, buffer.toString()); } (will commit that just now) > > > - It looks like you used JUnit 3 to write the tests - any reason we > wouldn't want to use JUnit 4? I just selected the wrong library when updating the project classpath - I did mean to select Junit 4, though I'm still not keen on the annotation version of JUnit which is why I've gone for extending TestCase and having a test method. Can change that if you want... ? Btw, I also just removed an uncessary 'unchecked' exception warning supression from QueryDictionaryTest, probably introduced as a result of not having to have an instance of Query to create a QueryDictionary. Cheers, Chris
