Differences between revisions 11 and 12
⇤ ← Revision 11 as of 2009-03-26 08:11:08 →
Size: 9733
Editor: EldZierau
Comment:
← Revision 12 as of 2009-03-26 09:35:04 → ⇥
Size: 9759
Editor: EldZierau
Comment:
Deletions are marked like this. Additions are marked like this.
Line 9: Line 9:
Line 14: Line 13:
Line 17: Line 15:
{{{ 
{{{
Line 20: Line 19:

'''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"
 '''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 27: Line 23:
Line 45: Line 42:
|| 342-346 || file != argument. Otherwise use ArgumentNotValid.checkNotNull(file, "..."); || Cosmetic || POSTPONED||
 || 342-346 || file != argument. Otherwise use ArgumentNotValid.checkNotNull(file, "..."); || Cosmetic || POSTPONED ||
Line 52: Line 48:
Line 54: Line 51:
Line 56: Line 54:
Line 58: Line 57:
Line 60: Line 60:
Line 62: Line 63:
Line 65: Line 67:
|| 115-117 || 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 70: Line 72:
Line 72: Line 75:
Line 74: Line 78:
Line 76: Line 81:
|| 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||		 || 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 79: Line 84:
|| 158-160 || These throws are not part of the method declaration || Cosmetic || SEE * ABOVE|| || 158-160 || These throws are not part of the method declaration || Cosmetic || SEE * ABOVE ||
Line 81: Line 86:
|| 190-192 || This throw is not part of the declaration of the method. || Cosmetic || SEE * ABOVE||
 || 190-192 || This throw is not part of the declaration of the method. || Cosmetic || SEE * ABOVE ||
Line 85: Line 89:
|| 110-111 || two empty line in a row || Cosmetic || NOTOK || || 110-111 || two empty line in a row || Cosmetic || OK ||
Line 91: Line 95:
|| 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 ||		 || 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 ||
Line 96: Line 100:
|| 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||		 || 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 ||
Line 99: Line 103:
|| 548 || This is not described in the method declaration. || Cosmetic || SEE * ABOVE|| || 548 || This is not described in the method declaration. || Cosmetic || SEE * ABOVE ||
Line 101: Line 105:
|| 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||		 || 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 ||
Line 105: Line 109:
|| 643-644 || throws not declared in method declaration || Cosmetic || SEE * ABOVE|| || 643-644 || throws not declared in method declaration || Cosmetic || SEE * ABOVE ||
Line 108: Line 112:
|| 668 || This throws is not described in the function declaration || Cosmetic || SEE * ABOVE|| || 668 || This throws is not described in the function declaration || Cosmetic || SEE * ABOVE ||
Line 116: Line 120:
Line 118: Line 123:
Line 120: Line 126:
Line 122: Line 129:
Line 132: Line 140:

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: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"

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

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

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