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 <uploadRetries>, rename <fileDir> to <baseFileDir>
/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