10354
Comment:
|
← Revision 14 as of 2010-08-16 10:24:42 ⇥
9774
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 9: | Line 9: |
Line 14: | Line 13: |
Line 17: | Line 15: |
{{{ time is registered under review 33 |
{{{ time is registered under review 37 |
Line 20: | Line 19: |
'''General comments''': || '''Description''' || '''Classification''' || '''Status''' || |
'''General comments''':<<BR>> * 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" |
Line 26: | Line 22: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 29: | Line 26: |
|| 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 || |
|| 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 || |
Line 38: | Line 35: |
|| 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 || |
|| 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 || |
Line 48: | Line 45: |
|| 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 || | || 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 || |
Line 50: | Line 47: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 52: | Line 50: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 54: | Line 53: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 56: | Line 56: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 58: | Line 59: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 60: | Line 62: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 63: | Line 66: |
|| 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 || |
|| 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 || |
Line 68: | Line 71: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 70: | Line 74: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 72: | Line 77: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 75: | Line 81: |
|| 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 || |
|| 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 || |
Line 83: | Line 89: |
|| 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 || |
|| 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 || |
Line 113: | Line 119: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 115: | Line 122: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 117: | Line 125: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 119: | Line 128: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 122: | Line 132: |
|| 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 || |
|| 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 || |
Line 127: | Line 137: |
|| 85 || 'body' should be of the type StringBuilder || Cosmetic || NOTOK || | || 85 || 'body' should be of the type StringBuilder || Cosmetic || POSTPONED || |
Line 129: | Line 139: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
Line 131: | Line 142: |
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' || | None |
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