Re: [Pki-devel] <pki-devel> [PATCH] 0005- Further work on TPS Processor, format operation.patch

On 3/17/2014 9:18 PM, John Magne wrote: Some comments/questions: 1. Is the TPSFormatProcessor still needed since the TPSSession can use TPSProcessor directly? Or maybe should TPSProcessor.format() be moved into TPSFormatProcessor? 2. As discussed over IRC, the TPSProcessor.format() right now is using two mechanisms to return the status: via exception and via return value. Will it be changed to use one mechanism in the future? Also already discussed, the code that checks the return value of the check*() method can actually be moved into the check*() method itself, then throw an TPSException with the status if it fails the validation. 3. The cuid_x variable in getTokenType() doesn't seem to be necessary. It looks like it can use cuid directly. 4. In the following code, the getString() will assign a default value if the property is missing, so the exception will not happen for missing property. try { mappingOrder = configStore.getString(configName, null); } catch (EBaseException e1) { //The whole feature won't work if this is wrong. throw new TPSException( "TPSProcessor.getTokenType: Token Type configuration incorrect! Mising mapping order!", TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND); } The exception could still happen for other errors, but in that case the exception message above will be incorrect. 5. The loop can be simplified like this: for (String mappingId : mappingOrder.split(",")) { ... } 6. Similar to #4, the following code will catch an incorrect type of error: try { targetTokenType = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { throw new TPSException( "TPSProcessor.getTokenType: Token Type configuration incorrect! Invalid or no targetTokenType.", TPSStatus.STATUS_ERROR_DEFAULT_TOKENTYPE_NOT_FOUND); } 7. Similar to #4, the following code will assign a default value if the property is missing. If there's an exception, it must have been caused by something else, not because of the missing property. In that case it's better to break and report the error. try { tokenType = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { tokenType = null; } try { tokenATR = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { tokenATR = null; } try { tokenCUIDStart = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { tokenCUIDStart = null; } try { tokenCUIDEnd = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { targetTokenType = null; } try { majorVersion = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { majorVersion = null; } try { minorVersion = configStore.getString(mappingConfigName, null); } catch (EBaseException e) { minorVersion = null; } The code should either wrap EBaseException with TPSException and throw it, or alternatively declare EBaseException in the getTokenType() and remove the try-catch block. 8. The following lines can be combined: int major = 0; major = Integer.parseInt(majorVersion); Same thing for the minor version. 9. There are checks in TPSProcessor to make sure the session is not null. This is no longer necessary since the session is supplied when creating the instance. If validation is necessary, it can be done just one time in the constructor itself: public TPSProcessor(TPSSession session) { if (session == null) throw NullPointerException("TPS session is null"); this.session = session; } -- Endi S. Dewata