9170
Comment:
|
9312
|
Deletions are marked like this. | Additions are marked like this. |
Line 63: | Line 63: |
|| 94-96 || These are not part of the method declaration. || Cosmetic || NOTOK || || 115-117 || These are not part of the method declaration. || Cosmetic || NOTOK || |
|| 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|| |
Line 75: | Line 75: |
|| 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 || |
|| 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|| |
Line 78: | Line 78: |
|| 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 || |
|| 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 || NOTOK || || 190-192 || This throw is not part of the declaration of the method. || Cosmetic || SEE * ABOVE|| |
Line 84: | Line 85: |
|| 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 || |
|| 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 || |
Line 91: | Line 92: |
|| 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 || |
|| 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 || |
Line 95: | Line 96: |
|| 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 || |
|| 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 || |
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 |
SEE * ABOVE |
115-117 |
These are not part of the method declaration. |
Cosmetic |
SEE * ABOVE |
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 |
SEE * ABOVE |
133-135 |
These throws are not part of method declaration. |
Cosmetic |
SEE * ABOVE |
137-139 |
ArgumentNotValid on these arguments. |
Cosmetic |
NOTOK |
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 |
NOTOK |
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 |
NOTOK |
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 |
NOTOK |
269-270 |
two empty lines in a row |
Cosmetic |
NOTOK |
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 |
NOTOK |
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 |
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