= 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. Almost done but still missing resolveBitarchiveID || Cosmetic || POSTPONED ||
|| General || Remove long lines on lines 137, 518, 664, 773-774, 834 and 941 || Cosmetic || OK ||
|| General || The class repeatedly uses "arcFileName", where indeed just "filename" would be adequate. No: it is important to distinguish between files that are batch (checksum) results and files that are uploaded || Cosmetic || REFUSED ||
|| 65 || Don't repeat "The Arcrepository". Replace with "This class" || Cosmetic || OK ||
|| 66 || "Succeeded Retrieval" => "Succeeded. Retrieval" || Cosmetic || OK ||
|| 68-69 || What is the definition of "an appropiate bitarchive"? || Cosmetic || POSTPONED ||
|| 69 || Missing punctuation: "bitarchive(s) Correction" => "bitarchive(s). Correction" || Cosmetic || OK ||
|| 101 || Remove TODO, or create feature request for this issue. || Cosmetic || OK ||
|| 124 || rename "con" as "jmsCon" || Cosmetic || OK ||
|| 133-134 || This is really the wrong exception for this problem. IllegalState should be thrown instead || Cosmetic || OK ||
|| 162-166 || This should not throw PermissionDenied, but IllegalState || Cosmetic || OK ||
|| 184-185 || Should be IllegalState instead of PermissionDenied || Cosmetic || OK ||
|| 211, 213, 215 || Spelling: "BitArhive" => "Bitarchive" || Cosmetic || OK ||
|| 231 || Check, if other sideeffects exist. || Cosmetic || OK ||
|| 240-241 || Are any of these allowed to be null here? No! They should all be ArgumentNotValid'ated. (CheckNotNull) || Cosmetic || OK ||
|| 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 || OK ||
|| 276 || Remove empty line || Cosmetic || OK ||
|| 293 || Language: "Upload started of file '" +  filename + "' to bitarchive '" + bitarchiveId + "'" || Cosmetic || OK ||
|| 561 || Spelling: legel => legal || Cosmetic || OK ||
|| 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 ||