Chris,
This looks great - thanks for jumping on it so quickly. I made a few minor
changes:
- Made QueryDictionary static (it wasn't using any of the outer class's
internal members).
- Moved the test app to pivot.web.test (I know there are valid reasons to put
it in pivot.web in the test project, but our current convention is to use a
test package - we'll probably revisit this later when we migrate all of our
tests to JUnit).
- Eliminated "left-hand literal" comparisons (e.g. "null == foo") - we don't
use this convention since inadvertent assignment isn't possible in Java.
- Naming convention (minor): we generally use "previous" rather than "old" to
refer to previous values.
- Minor formatting changes in Query.java.
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));
}
}
- It looks like you used JUnit 3 to write the tests - any reason we wouldn't
want to use JUnit 4?
Thanks,
Greg
On Sunday, May 24, 2009, at 02:08PM, "Christopher Brind" <[email protected]>
wrote:
>Hi,
>
>That's in now. Let me know what you think.
>
>Cheers,
>Chris
>
>
>2009/5/24 Christopher Brind <[email protected]>
>
>> Hi,
>>
>> That all makes sense. I should be able to commit something in the next few
>> hours.
>>
>> Cheers,
>> Chris
>>
>>
>>
>> 2009/5/23 Greg Brown <[email protected]>
>>
>> >When I thought about it some more it seemed to me that we needed a couple
>>> of
>>> >extra methods on the QueryDictionary in addition to what we discussed
>>> >earlier.
>>>
>>> I came to the same conclusion myself this morning. Here's what I came up
>>> with. It's similar to what you proposed, but may be slightly more consistent
>>> with Pivot's existing collection APIs. It basically merges the Dictionary
>>> and Sequence interfaces:
>>>
>>> public interface QueryDictionary
>>> extends Dictionary<String, String>, Iterable<String> {
>>> public String get(String key); // from Dictionary
>>> public String get(String key, int index); // new
>>>
>>> public String put(String key, String value); // from Dictionary
>>> public int add(String key, String value); // new
>>> public void insert(String key, String value, int index); // new
>>>
>>> public String remove(String key); // from Dictionary
>>> public String remove(String key, int index); // new
>>>
>>> public void clear(); // new
>>>
>>> public boolean containsKey(String key); // from Dictionary
>>> public boolean isEmpty(); // from Dictionary
>>>
>>> public int getLength(String key); // new
>>> }
>>>
>>> >It also seemed necessary to clarify what the implications of this
>>> >interface have on the Dictionary that it implements. Namely, how the
>>> #put
>>> >and #get methods behave based on the fact that the backing storage for
>>> >values is now a sequence. (Hope that makes sense)
>>>
>>> Agreed. I think the behavior you documented for these methods is what
>>> developers would expect.
>>>
>>> >Also, while this is an interface the implementation details are pretty
>>> much
>>> >tied down, so shouldn't this be a concrete class that the other classes
>>> >extend instead? They are marked as final so it isn't as though any
>>> further
>>> >inheritence would be possible.
>>>
>>> Actually, I'm thinking it could just be a concrete class backed by an
>>> instance of HashMap<String, ArrayList<String>>. We could try to enforce
>>> read-only access for the response data if we wanted to, but right now I'm
>>> leaning towards keeping it simple and making it read/write. We'd end up with
>>> three instances of HashMap<String, ArrayList<String>> and three
>>> corresponding instances of QueryDictionary to wrap them.
>>>
>>> Thoughts?
>>>
>>> Greg
>>>
>>>
>>>
>>
>
