Hi Pavel,
>> 215 .append("
>> rem=").append(b.remaining()).append("]").toString();
>>
>> Please use ']' instead of "]”.
>>
>> 222
>> .append("[len=").append(s.length()).append("]").toString();
>>
>> Please use ']' instead of "]”.
>
> I believe since JEP 280 [1] is already here, we don't need to bother about
> this
> kind of things any more (generally speaking).
Static code analyzers produce a warning for code like this.
Btw. in line 212
212 return new StringBuffer()
you use StringBuffer instead of StringBuilder.
JEP-280 does not apply in this case. But when you rewrite this code to
use the plain String concatenation, then JEP-280 would apply:
211 static String toString(Buffer b) {
212 return toStringSimple(b)
214 + "[pos=“ + b.position()
215 + " lim=“ + b.limit()
216 + " cap=“ + b.capacity()
217 + " rem=“ + b.remaining() + "]";
218 }
This code is easier to read than the one with StringBuilder, at least for me.
>> 177 throw new
>> IllegalArgumentException(String.valueOf(n));
>>
>> Could you please provide a better message for exception here?
>
> What would the purpose be? It's an internal implementation detail. If this
> exception is thrown, it means it's a bug in the implementation. And I highly
> doubt this error message would be of any help to an end-user. It's not
> something
> they can influence/change (most likely).
> What's important here is the stack-trace and te value. Maybe I will include
> more
> state to be printed.
A good error message is gold worth. It may save you hours of debugging.
Therefore if you can add more information to the exception message, then please
do it as it will help you to find and to fix bugs faster.
>
>> src/java.httpclient/share/classes/java/net/http/SignalHandler.java
>>
>> Maybe the lines 33-46 should be converted to a JavaDocs comment.
>>
>> 112 switch (s) {
>> 113 case RUNNING:
>> 114 return RERUN;
>> 115 case DONE:
>> 116 return RUNNING;
>> 117 case RERUN:
>> 118 return RERUN;
>> 119 default:
>> 120 throw new InternalError(String.valueOf(s));
>> 121 }
>>
>> Please fix the indentation.
>
> I've tried to adhere to this:
>
> http://cr.openjdk.java.net/~alundblad/styleguide/#toc-indentation
>
> Might revisit later. I will wait for the status change of this document.
Btw. have you considered to use an array instead of switch-statement?
The array may look like this:
private static final int[] NEXT_STATE = new int[] {
RUNNING, // DONE -> RUNNING
RERUN, // RUNNING -> RERUN
RERUN, // RERUN -> RERUN
};
And then the line 111 can be written as:
111 int prev = state.getAndUpdate(s -> NEXT_STATE[s]);
The only advantage is that it produces less byte codes.
But maybe it is still worth.
>
>> src/java.httpclient/share/classes/java/net/http/WebSocket.java
>>
>> Could you please add an #of-method to CloseCode which also takes
>> a description as parameter, e.g.:
>
> I thought about it. But then we might have an ambiguity. One might create
> different versions of the object with the same numerical code, but different
> descriptions. Should they be equal?
Yes.
> If they are gonna be cached, then which one?
CloseCodes created by #of-method should never be cached by JDK.
> Overall, it might kill this class' property of being value-based.
>
> If a non-standard Close Code would be created, it would be created by the
> client, rather than by the server. So a client already knows what the code
> means.
My main motivation to have a description for custom CloseCodes is
only to simplify debugging. So when you think it is overkill and not needed,
then it is OK for me to leave the code for now as is.
I have already started to port our code to use the new API and I have one
comment.
I think you should introduce a new class CloseReason as it was done in JSR 356
(http://docs.oracle.com/javaee/7/api/javax/websocket/CloseReason.html)
and use it in #sendClose and #onClose-methods. The #onClose-method in the
WebSocket.Listener interface looks really ugly:
onClose(WebSocket webSocket, Optional<WebSocket.CloseCode> code, String reason)
The parameter “reason” makes only sense when code.isPresent() returns true.
The code and the reason belong together. But the one is wrapped in Optional and
the other not.
It doesn’t feel right. But when you introduce the new class CloseReason, then
you can change this method to:
onClose(WebSocket webSocket, Optional<WebSocket.CloseReason> closeReason)
I think it makes the API much better and it is clear that the reason phrase is
only
available when you have a close code too.
And I think there should be a way to configure a timeout for sending messages.
One minor thing off-topic:
While changing my code I noticed that the class
ProxySelector$StaticProxySelector
and the JavaDocs of the ProxySelector#of-method can be improved a little bit:
diff -r 589795e4cd38 src/java.base/share/classes/java/net/ProxySelector.java
--- a/src/java.base/share/classes/java/net/ProxySelector.java Wed Mar 23
19:33:39 2016 -0700
+++ b/src/java.base/share/classes/java/net/ProxySelector.java Mon Mar 28
19:30:24 2016 +0200
@@ -165,7 +165,8 @@
/**
* Returns a ProxySelector which uses the given proxy address for all HTTP
- * and HTTPS requests. If proxy is {@code null} then proxying is disabled.
+ * and HTTPS requests. If the given proxy address is {@code null}
+ * then proxying is disabled.
*
* @param proxyAddress
* The address of the proxy
@@ -178,9 +179,9 @@
return new StaticProxySelector(proxyAddress);
}
- static class StaticProxySelector extends ProxySelector {
+ private static final class StaticProxySelector extends ProxySelector {
private static final List<Proxy> NO_PROXY_LIST =
List.of(Proxy.NO_PROXY);
- final List<Proxy> list;
+ private final List<Proxy> list;
StaticProxySelector(InetSocketAddress address){
Proxy p;
@@ -198,9 +199,9 @@
}
@Override
- public synchronized List<Proxy> select(URI uri) {
- String scheme = uri.getScheme().toLowerCase();
- if (scheme.equals("http") || scheme.equals("https")) {
+ public List<Proxy> select(URI uri) {
+ String scheme = uri.getScheme();
+ if (scheme.equalsIgnoreCase("http") ||
scheme.equalsIgnoreCase("https")) {
return list;
} else {
return NO_PROXY_LIST;
Best regards,
Andrej Golovnin
