= Review (NS-36): Settings and channel changes, part two = || Author || Eld Zierau || || Moderator || Eld Zierau || || State || Closed || == Objectives == {{{ Additional to review NS-33: Mostly refactoring, but also two below. Special for archive settings: new setting , rename to /trunk/src/dk/netarkivet/archive/settings.xml /trunk/src/dk/netarkivet/archive/ArchiveSettings.java /trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java line 835 }}} '''Total Time Used (Coding,Documentation,Review)''': {{{ time is registered under review 37 }}} '''General comments''':<
> * Several comments concerns throws in signature of functions. The code standard does not address this, and there are different approaches. Nothing should be corrected in this respect before we have decided on approach and described it in the coding standards. The comments in question is marked with: "SEE * ABOVE" === Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/JMSConnectionSunMQ.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 79, 85, 91 || public, private or protected? - check this || Cosmetic || POSTPONED || || 120, 123 || Add to javadoc: @throws UnknownID ... || Cosmetic || POSTPONED || || 152-154 || Remove out commented code. || Cosmetic || OK || || 178 || "1" should be a constant. || Cosmetic || POSTPONED || || 179-181 || Remove outcommented code. || Cosmetic || OK || || 273 || Odd empty line || Cosmetic || OK || || 346 || odd empty line. || Cosmetic || OK || === Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestClient.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 170, 186 || "," should be a constant || Cosmetic || POSTPONED || || 200-201 || two empty lines in a row. || Cosmetic || OK || || 254 || ".gz" should be a constant. || Cosmetic || POSTPONED || || 298, 305, 311, 318, 325, 327, 338, 345 || "," should be constant. || Cosmetic || POSTPONED || || 315-320 || 'foundJobs' is not an argument for this function -> thus improper ArgumentNotValid. Otherwise use ArgumentNotValid.checkNotNull(foundJobs, "..."); || Cosmetic || POSTPONED || || 322-328 || Again ArgumentNotValid on a variable (not an argument). Otherwise use ArgumentNotValid.checkTrue(jobSet.containsAll(foundJobs), "..."); || Cosmetic || POSTPONED || || 335-339 || Files != argument. Otherwise use ArgumentNotValid.checkNotNull(files, "..."); || Cosmetic || POSTPONED || || 342-346 || file != argument. Otherwise use ArgumentNotValid.checkNotNull(file, "..."); || Cosmetic || POSTPONED || === Comments on file 'trunk/src/dk/netarkivet/harvester/HarvesterSettings.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || General || Most of the comments have been change from ending with the line: */ to ending with the '*/' at the same line as the comments. Why divert from our standard code comment style ? Answer is done for all javadoc regarding settings, if changed back it should be done in all java setting files. || Cosmetic || POSTPONED || === Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClientSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/monitor/settings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/CommonSettings.java', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/HTTPRemoteFileSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/archive/ArchiveSettings.java', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/Constants.java', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/HTTPSRemoteFile.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 94-96 || These are not part of the method declaration. || Cosmetic || SEE * ABOVE || || 115-117 || These are not part of the method declaration. || Cosmetic || SEE * ABOVE || || 119 || check for ArgumentNotValid on 'File f'. || Cosmetic || POSTPONED || || 125-126, 136-137 || Two empty lines in a row. || Cosmetic || OK || === Comments on file 'trunk/src/dk/netarkivet/common/distribute/JMSConnectionSunMQSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/HTTPSRemoteFileSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/FTPRemoteFileSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/HTTPRemoteFile.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 96-98 || These throws are not part of the method declaration. || Cosmetic || SEE * ABOVE || || 133-135 || These throws are not part of method declaration. || Cosmetic || SEE * ABOVE || || 137-139 || ArgumentNotValid on these arguments. || Cosmetic || POSTPONED || || 158-160 || These throws are not part of the method declaration || Cosmetic || SEE * ABOVE || || 165-171 || ArgumentNotValid.checkTrue( (destFile.isFile() "or" destFile.canWrite()) && (destFile.getParentFile().isDirectory() "or" destFile.getParentFile().canWrite()), " ... "); Perhaps split into two. || Cosmetic || POSTPONED || || 190-192 || This throw is not part of the declaration of the method. || Cosmetic || SEE * ABOVE || === Comments on file 'trunk/src/dk/netarkivet/common/webinterface/HTMLUtils.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 110-111 || two empty line in a row || Cosmetic || OK || || 118 || ArgumentNotValid.checkNotNullOrEmpty(s) ? || Cosmetic || POSTPONED || || 120, 133 || "UTF-8" should be a constant. || Cosmetic || POSTPONED || || 131 || ArgumentNotValid.checkNotNullOrEmpty(s) ? || Cosmetic || POSTPONED || || 147 || ArgumentNotValid.checkNotNull(context) ? || Cosmetic || POSTPONED || || 163 || ArgumentNotValid.checkNotNull(context) || Cosmetic || POSTPONED || || 232-233 || Why use two lines on less than one line of code? || Cosmetic || OK || || 269-270 || two empty lines in a row || Cosmetic || OK || || 303 || ArgumentNotValid.checkNotNull(contents); || Cosmetic || POSTPONED || || 433, 435 || "UTF-8" as a constant. || Cosmetic || POSTPONED || || 437-438 || This seems to be an IlligalState, not ArgumentNotValid. || Cosmetic || POSTPONED || || 474 || Why use the values '6' and '3' ??? Change to constants. || Cosmetic || POSTPONED || || 511 || This is not described in the method declaration. || Cosmetic || SEE * ABOVE || || 513-515 || ArgumentNotValid on all these arguments. || Cosmetic || POSTPONED || || 548 || This is not described in the method declaration. || Cosmetic || SEE * ABOVE || || 550-552 || ArgumentNotValid on all these arguments. || Cosmetic || POSTPONED || || 585 || This is not described in the function declaration. || Cosmetic || SEE * ABOVE || || 587-589 || ArgumentNotValid on all these arguments. || Cosmetic || POSTPONED || || 617 || This throws are not declared in the declaration of the function. || Cosmetic || SEE * ABOVE || || 620-621 || ArgumentNotValid on these arguments. || Cosmetic || POSTPONED || || 643-644 || throws not declared in method declaration || Cosmetic || SEE * ABOVE || || 646-647 || ArgumentNotValid on these arguments. || Cosmetic || POSTPONED || || 652 || use value.trim().isEmpty() instead ? || Cosmetic || POSTPONED || || 668 || This throws is not described in the function declaration || Cosmetic || SEE * ABOVE || || 671-673 || ArgumentNotValid on these arguments. || Cosmetic || POSTPONED || || 700-702 || ArgumentNotValid on these arguments. || Cosmetic || POSTPONED || || 737-738 || ArgumentNotValid on 'context' and 'defaultValue'. || Cosmetic || POSTPONED || || 769-770 || ArgumentNotValid on 'context'. || Cosmetic || POSTPONED || || 774 || replace 'paramValue.trim().length() > 0' with '!paramValue.trim().isEmpty()' ? || Cosmetic || POSTPONED || === Comments on file 'trunk/src/dk/netarkivet/harvester/settings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestClientSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/viewerproxy/ViewerProxySettings.java', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/monitor/MonitorSettings.java', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/common/distribute/FTPRemoteFile.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 173 || "-" as a constant. E.g. FTP_DEFAULT_FILE_NAME || Cosmetic || POSTPONED|| || 179-267 || This is too confusing. else { try { while { try { if { ?? Restructure and make methods for subroutines. || Cosmetic || POSTPONED || || 456, 470, 495 || "." and "*" as constants. || Cosmetic || POSTPONED || === Comments on file 'trunk/src/dk/netarkivet/common/utils/EMailNotifications.java', revision 637 === || '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || || 85 || 'body' should be of the type StringBuilder || Cosmetic || POSTPONED || === Comments on file 'trunk/src/dk/netarkivet/common/utils/EMailNotificationsSettings.xml', revision 637 === None === Comments on file 'trunk/src/dk/netarkivet/viewerproxy/settings.xml', revision 637 === None