[Pki-devel] [PATCH] 862 HttpConnFactory addendum

[Endi Sukma Dewata]

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-arraylist-and-vector.html

http://stackoverflow.com/questions/1386275/why-is-java-vector-class-considered-obsolete-or-deprecated

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