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:
Description |
Classification |
Status |
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? |
Cosmetic |
NOTOK |
120, 123 |
Add to javadoc: @throws UnknownID ... |
Cosmetic |
NOTOK |
152-154 |
Remove out commented code. |
Cosmetic |
NOTOK |
178 |
"1" should be a constant. |
Cosmetic |
NOTOK |
179-181 |
Remove outcommented code. |
Cosmetic |
NOTOK |
273 |
Odd empty line |
Cosmetic |
NOTOK |
346 |
odd empty line. |
Cosmetic |
NOTOK |
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 |
NOTOK |
200-201 |
two empty lines in a row. |
Cosmetic |
NOTOK |
254 |
".gz" should be a constant. |
Cosmetic |
NOTOK |
298, 305, 311, 318, 325, 327, 338, 345 |
"," should be constant. |
Cosmetic |
NOTOK |
315-320 |
'foundJobs' is not an argument for this function -> thus improper ArgumentNotValid. Otherwise use ArgumentNotValid.checkNotNull(foundJobs, "..."); |
Cosmetic |
NOTOK |
322-328 |
Again ArgumentNotValid on a variable (not an argument). Otherwise use ArgumentNotValid.checkTrue(jobSet.containsAll(foundJobs), "..."); |
Cosmetic |
NOTOK |
335-339 |
Files != argument. Otherwise use ArgumentNotValid.checkNotNull(files, "..."); |
Cosmetic |
NOTOK |
342-346 |
file != argument. Otherwise use ArgumentNotValid.checkNotNull(file, "..."); |
Cosmetic |
NOTOK |
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 ? |
Cosmetic |
NOTOK |
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 |
NOTOK |
115-117 |
These are not part of the method declaration. |
Cosmetic |
NOTOK |
119 |
check for ArgumentNotValid on 'File f'. |
Cosmetic |
NOTOK |
125-126, 136-137 |
Two empty lines in a row. |
Cosmetic |
NOTOK |
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 |
NOTOK |
||
133-135 |
These throws are not part of method declaration. |
Cosmetic |
NOTOK |
||
137-139 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
||
158-160 |
These throws are not part of the method declaration |
Cosmetic |
NOTOK |
||
165-171 |
ArgumentNotValid.checkTrue( (destFile.isFile() |
destFile.canWrite()) && (destFile.getParentFile().isDirectory() |
destFile.getParentFile().canWrite()), " ... "); Perhaps split into two. |
Cosmetic |
NOTOK |
190-192 |
This throw is not part of the declaration of the method. |
Cosmetic |
NOTOK |
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 |
NOTOK |
118 |
ArgumentNotValid.checkNotNullOrEmpty(s) ? |
Cosmetic |
NOTOK |
120, 133 |
"UTF-8" should be a constant. |
Cosmetic |
NOTOK |
131 |
ArgumentNotValid.checkNotNullOrEmpty(s) ? |
Cosmetic |
NOTOK |
147 |
ArgumentNotValid.checkNotNull(context) ? |
Cosmetic |
NOTOK |
163 |
ArgumentNotValid.checkNotNull(context) |
Cosmetic |
NOTOK |
232-233 |
Why use two lines on less than one line of code? |
Cosmetic |
NOTOK |
269-270 |
two empty lines in a row |
Cosmetic |
NOTOK |
303 |
ArgumentNotValid.checkNotNull(contents); |
Cosmetic |
NOTOK |
433, 435 |
"UTF-8" as a constant. |
Cosmetic |
NOTOK |
437-438 |
This seems to be an IlligalState, not ArgumentNotValid. |
Cosmetic |
NOTOK |
474 |
Why use the values '6' and '3' ??? Change to constants. |
Cosmetic |
NOTOK |
511 |
This is not described in the method declaration. |
Cosmetic |
NOTOK |
513-515 |
ArgumentNotValid on all these arguments. |
Cosmetic |
NOTOK |
548 |
This is not described in the method declaration. |
Cosmetic |
NOTOK |
550-552 |
ArgumentNotValid on all these arguments. |
Cosmetic |
NOTOK |
585 |
This is not described in the function declaration. |
Cosmetic |
NOTOK |
587-589 |
ArgumentNotValid on all these arguments. |
Cosmetic |
NOTOK |
617 |
This throws are not declared in the declaration of the function. |
Cosmetic |
NOTOK |
620-621 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
643-644 |
throws not declared in method declaration |
Cosmetic |
NOTOK |
646-647 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
652 |
use value.trim().isEmpty() instead ? |
Cosmetic |
NOTOK |
668 |
This throws is not described in the function declaration |
Cosmetic |
NOTOK |
671-673 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
700-702 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
737-738 |
ArgumentNotValid on 'context' and 'defaultValue'. |
Cosmetic |
NOTOK |
769-770 |
ArgumentNotValid on 'context'. |
Cosmetic |
NOTOK |
774 |
replace 'paramValue.trim().length() > 0' with '!paramValue.trim().isEmpty()' ? |
Cosmetic |
NOTOK |
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 |
NOTOK |
179-267 |
This is too confusing. else { try { while { try { if { ?? Restructure and make methods for subroutines. |
Cosmetic |
NOTOK |
456, 470, 495 |
"." and "*" as constants. |
Cosmetic |
NOTOK |
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 |
NOTOK |
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