Re: [Pki-devel] [PATCH] Ticket #888 - TPS rewrite: provide remote authority functions (patch 1: TKS functions)

On 3/14/2014 1:42 PM, Christina Fu wrote: > Thank you Endi and Jack for your comments. > Attached please find the patch that addressed your comments. > > thanks, > Christina Just a few more comments: 1. The computeSessionKey() will throw an exception if it's missing the RESPONSE_STATUS. The other methods will just ignore that. I think they should be consistent. 2. Right now the code gets a string value from the response object, decodes the value into TPSBuffer, then puts it back to the same response object under the same name. Then the new Response classes would read from the same response object again. It works fine but code maintainer would have to be aware of the current type of the attribute because it could change. // response contains String or TPSBuffer values Hashtable<String, Object> response = parseResponse(content); value = (String) response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) { CMS.debug(...); } else { CMS.debug(...); response.put(IRemoteRequest.TKS_RESPONSE_SessionKey, Util.specialDecode(value)); } return new TKSComputeSessionKeyResponse(response); It might be easier to simply wrap the original response object with the new Response class, then decode the value only when the attribute is used: // response contains string values Hashtable<String, String> response = parseResponse(content); return new TKSComputeSessionKeyResponse(response); public TPSBuffer getSessionKey() { String value = response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) return null; // decode value only when needed return Util.specialDecode(value)); } Or alternatively the new Response class can hold just the decoded values either in a separate hashtable or in fields: Hashtable<String, String> response = parseResponse(content); TKSComputeSessionKeyResponse res = new TKSComputeSessionKeyResponse(); // don't use response value = response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) { CMS.debug(...); } else { CMS.debug(...); // store decoded value res.setSessionKey(Util.specialDecode(value)); } return res; Issue #1 I think should be fixed. ACK once that is addressed. For #2, it's not a problem, but it would improve code clarity. I'll let you decide. Thanks.