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

IssuesFoundInReviewNs34 (last edited 2010-08-16 10:24:42 by localhost)