[#879] Additional renaming and checkstyle changes #886
Conversation
…dability or conformity to documentation
| @@ -526,14 +522,18 @@ private void processTpmEvents(final List<SupportReferenceManifest> dbSupportRims | |||
|
|
|||
| this.referenceDigestValueRepository.save(newRdv); | |||
| } | |||
| } catch (CertificateException | NoSuchAlgorithmException | IOException e) { | |||
| } catch (CertificateException e) { | |||
There was a problem hiding this comment.
Since we are printing the same stack trace for all three exceptions, why did you separate them out into their own catch statement? Do you believe that we will add more code to each exception case?
There was a problem hiding this comment.
This change was automatically applied by checkstyles. There are instances in the tcg_rim_tool of exceptions broken out into distinct catch blocks for specific handling, which can be applied here if appropriate.
| @@ -608,6 +608,12 @@ public ModelAndView initPage(final ReferenceManifestDetailsPageParams params, | |||
| String uuidError = "Failed to parse ID from: " + params.getId(); | |||
| messages.addError(uuidError); | |||
| log.error(uuidError, iaEx); | |||
| } catch (CertificateException cEx) { | |||
There was a problem hiding this comment.
I noticed that we are catching every possible exception the application could throw at us (with the very last catch statement on line 617 in the modified file), is that last catch necessary? If it is necessary, do you believe it would make sense to collapse all of the above exceptions into one big catch seeing as that we are logging the same exception and we aren't doing anything special per exception?
There was a problem hiding this comment.
Exception handling can be more elegantly handled in the ACA. In many cases a try block is followed by several catches with unique parameters that are otherwise identical. If deemed appropriate these can be addressed in a future PR.
There was a problem hiding this comment.
Yeah this issue isn't unique to this file. I think addressing our way of handling exceptions in a future PR is the move.
| final PolicySettings policySettings) { | ||
| List<TpmPcrEvent> tpmPcrEvents = new LinkedList<>(); | ||
| for (TpmPcrEvent tpe : tcgMeasurementLog.getEventList()) { | ||
| if (policySettings.isIgnoreImaEnabled() && tpe.getPcrIndex() == IMA_PCR) { | ||
| log.info("IMA Ignored -> {}", tpe); | ||
| log.info(String.format("IMA Ignored -> %s", tpe)); |
There was a problem hiding this comment.
I've noticed that you've changed the parameterized log messages to string format messages throughout the three files. Is this the standard that we are supposed to follow or is there another reason for this change?
There was a problem hiding this comment.
This change was automatically applied by checkstyles so I assumed that was the standard we were following. I can look into logging best practices to see if there's any reason to prefer one over the other.
There was a problem hiding this comment.
No worries! I wasn't trying to be nit-picky. I wasn't entirely sure if this was the standard we wanted to stick to or if parameterized log messages was the way to go. I saw that the Intellij IDE suggested that as the "preferred" option; however I do not know what the team believes is best.
These changes follow those from #848. It should be noted that after reviewing the documentation the ReferenceDigestValue class was deemed appropriately named and not renamed as originally intended. Variable names in a few other classes have been refactored to more clearly align with TCG documentation. More checkstyle changes have been implemented as well.
Closes #879