NetarchiveSuite|
Size: 3859
Comment:
|
Size: 3834
Comment:
|
| Deletions are marked like this. | Additions are marked like this. |
| Line 43: | Line 43: |
| || 566-567 || Should we really accept a null value for checksumResFile? Doesn't it represent a problem in our programming logic Add to log if the checksumResFile was null. || Cosmetic || NOTOK || || 652 || Spelling: Readen => read || Cosmetic || NOTOK || || 730 || Missing punctuation: "the bitarchive This value" => "the bitarchive. This value" || Cosmetic || NOTOK || || 733 || Spelling: readen => read || Cosmetic || NOTOK || || 748, 753 || Code style violation: A line must not end with a "+" or "&&" or "||" || Cosmetic || NOTOK || || 754 || Can be written as: !reportedChecksum.isEmpty() || Cosmetic || NOTOK || || 768 || can be written as Can be written as: reportedChecksum.isEmpty() || Cosmetic || NOTOK || || 783, 789 || Code style violation: "+" at end of line. || Cosmetic || NOTOK || |
|| 566-567 || Should we really accept a null value for checksumResFile? Doesn't it represent a problem in our programming logic Add to log if the checksumResFile was null. || Cosmetic || OK || || 652 || Spelling: Readen => read || Cosmetic || OK || || 730 || Missing punctuation: "the bitarchive This value" => "the bitarchive. This value" || Cosmetic || OK || || 733 || Spelling: readen => read || Cosmetic || OK || || 748, 753 || Code style violation: A line must not end with a "+" or "&&" or ... || Cosmetic || OK || || 754 || Can be written as: !reportedChecksum.isEmpty() || Cosmetic || OK || || 768 || can be written as Can be written as: reportedChecksum.isEmpty() || Cosmetic || OK || || 783, 789 || Code style violation: "+" at end of line. || Cosmetic || OK || |
Review (NS-34): Review of bug 574 (we do strange things on wrong results)
Author |
Eld Zierau |
Moderator |
Eld Zierau |
State |
Closed |
Objectives
/trunk/src/dk/netarkivet/archive/ArchiveSettings.java 60-67 /trunk/src/dk/netarkivet/archive/settings.xml 36 /trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java All
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) ELZI: 3 MD SVC: 0.5 MD
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 738
Lines |
Description |
Classification |
Status |
General |
Missing javadoc for private methods starting on lines: 417,427, and 460 |
Cosmetic |
NOTOK |
General |
Remove long lines on lines 137, 518, 664, 773-774, 834 and 941 |
Cosmetic |
NOTOK |
General |
The class repeatedly uses "arcFileName", where indeed just "filename" would be adequate. |
Cosmetic |
NOTOK |
65 |
Don't repeat "The Arcrepository". Replace with "This class" |
Cosmetic |
NOTOK |
66 |
"Succeeded Retrieval" => "Succeeded. Retrieval" |
Cosmetic |
NOTOK |
68-69 |
What is the definition of "an appropiate bitarchive"? |
Cosmetic |
NOTOK |
69 |
Missing punctuation: "bitarchive(s) Correction" => "bitarchive(s). Correction" |
Cosmetic |
NOTOK |
101 |
Remove TODO, or create feature request for this issue. |
Cosmetic |
NOTOK |
124 |
rename "con" as "jmsCon" |
Cosmetic |
NOTOK |
133-134 |
This is really the wrong exception for this problem. IllegalState should be thrown instead |
Cosmetic |
NOTOK |
162-166 |
This should not throw PermissionDenied, but IllegalState |
Cosmetic |
NOTOK |
184-185 |
Should be IllegalState instead of PermissionDenied |
Cosmetic |
NOTOK |
211, 213, 215 |
Spelling: "BitArhive" => "Bitarchive" |
Cosmetic |
NOTOK |
231 |
Check, if other sideeffects exist. |
Cosmetic |
NOTOK |
240-241 |
Are any of these allowed to be null here? No! They should all be ArgumentNotValid'ated. (CheckNotNull) |
Cosmetic |
NOTOK |
248 |
Should we check here, if the filename already exists in the outstandingRemoteFiles map? If not, add comment, that it is ok to overwrite existing remoteFile in the outstandingRemoteFiles map Consider logging, if already in map. |
Cosmetic |
NOTOK |
276 |
Remove empty line |
Cosmetic |
NOTOK |
293 |
Language: "Upload started of file '" + filename + "' to bitarchive '" + bitarchiveId + "'" |
Cosmetic |
NOTOK |
561 |
Spelling: legel => legal |
Cosmetic |
NOTOK |
566-567 |
Should we really accept a null value for checksumResFile? Doesn't it represent a problem in our programming logic Add to log if the checksumResFile was null. |
Cosmetic |
OK |
652 |
Spelling: Readen => read |
Cosmetic |
OK |
730 |
Missing punctuation: "the bitarchive This value" => "the bitarchive. This value" |
Cosmetic |
OK |
733 |
Spelling: readen => read |
Cosmetic |
OK |
748, 753 |
Code style violation: A line must not end with a "+" or "&&" or ... |
Cosmetic |
OK |
754 |
Can be written as: !reportedChecksum.isEmpty() |
Cosmetic |
OK |
768 |
can be written as Can be written as: reportedChecksum.isEmpty() |
Cosmetic |
OK |
783, 789 |
Code style violation: "+" at end of line. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/ArchiveSettings.java', revision 675
Lines |
Description |
Classification |
Status |
60 |
Is this a new setting? If yes, this should be mentioned in the releasenotes |
Cosmetic |
OK |