On 3/24/2014 2:10 PM, Christina Fu wrote:
The HttpConnFactory was apparently missed in #862 for multi-uri support. This patch adds such support. Please note that in order to add multi-uri support, the maximum connection is turned "soft" limit to accommodate the likely case that when the max has reached but no existing connection for a needed uri is present. To support this, the original Java Array is replaced by Vector instead for mConns so that the size can be flexible and increased to exceed the max when needed. The existing service for KRA was tested as well as the TPS (TKS, CA and mixed uri's) cases.
Please review.
thanks, Christina
Some comments: 1. New codes should be written using Java collection framework (i.e. ArrayList) instead of Vector because it's lightweight, concise, more efficient, and easier to use with other collections. Vector is a legacy class and synchronized by default, but in most cases synchronization is not necessary or already handled by the class using the Vector. And if necessary, ArrayList can be synchronized as well so there's no need to use Vector unless for backward compatibility. Please see the following pages: http://javapostsforlearning.blogspot.com/2013/02/difference-between-arraylis... http://stackoverflow.com/questions/1386275/why-is-java-vector-class-consider... 2. In HttpConnFactory.createConnection() it's not necessary to check whether retConn is null because the CMSEngine.getHttpConnection() will create a new instance which is never null. If it can't create a new instance it will throw an exception. 3. In HttpConnFactory.getConnForOp() there seems to be a possibility the mNumConns and mConns would no longer synced up. // the mNumConns is decreased here mNumConns--; for (int i = 0; i <= mNumConns; i++) { if (...) { mConns.removeElementAt(i); } else { // but here the mConns is not changed } } I'd suggest removing mNumConns and using mConns.size() instead. 4. In getConnForOp() if op is null it will remove an element but will still return a null: private IHttpConnection getConnForOp(String op) { IHttpConnection retConn = null; if (op == null) { retConn = mConns.elementAt(mNumConns); mConns.removeElementAt(mNumConns); } else { ... } return retConn; // return null? } Is this the correct behavior? Or should it return the removed element? To reduce possible similar errors I'd suggest in general to return from the method as soon as early as possible: private IHttpConnection getConnForOp(String op) { if (op == null) { retConn = mConns.elementAt(mNumConns); mConns.removeElementAt(mNumConns); return null; // return immediately } // otherwise continue with the method IHttpConnection retConn = null; ... } 5. In isConnForOp() and getConnForOp() the same element is being looked up multiple times: for (int i = 0; i <= mNumConns; i++) { if ((mConns.elementAt(i).getOp() != null) && op.equals(mConns.elementAt(i).getOp())) { ... } else { if (mConns.elementAt(i).getOp() != null) { ... It would be more efficient and easier to read to do the lookup once then reuse it: for (int i = 0; i <= mNumConns; i++) { String connOp = mConns.get(i).getOp(); if (connOp != null && op.equals(connOp)) { ... } else { if (connOp != null) { ... It's also possible to use for-each statement: for (IHttpConnection conn : mConns) { String connOp = conn.getOp(); if (connOp != null && op.equals(connOp)) { ... } else { if (connOp != null) { ... 6. The boolean literal comparison is not necessary: isConnForOp(op) == false It can be simplified as follows: !isConnForOp(op) The not (!) operator can only be used with boolean. It cannot be used to check null pointer, so there shouldn't be any ambiguity. 7. The following line: mConns.add(mNumConns++, boundconn); can be replaced with: mConns.add(boundconn); because List.add() will always add the element to the end of the list. 8. In HttpConnection the m-prefix and the null initialization is not necessary for the new attribute. Generally it's expected the attribute will have the same name as the setter/getter methods, and all attributes are null by default. protected String op; // null by default public String getOp() { return op; } public HttpConnection(..., String op) { this.op = op; } -- Endi S. Dewata