Review (NS-129): Changes caused by progress in Assignment B.2.2b.

Author

Jonas

Moderator

Jonas

State

Closed

Description

Classification

Status

Consider starting the all // comments with a capital letter. Currently this is not done consistently.

Cosmetic

NOTOK

Lines

Description

Classification

Status

112

Consider synchronizing the method

Cosmetic

NOTOK

Lines

Description

Classification

Status

439-440

The name and the type should not be the same.

Cosmetic

NOTOK

Lines

Description

Classification

Status

130

Consider removing validation. Validation is only required for public methods

Cosmetic

NOTOK

298

Change to: if (referenceCheckSum.isEmpty())

Cosmetic

NOTOK

Lines

Description

Classification

Status

General

Missing svn:keywords property with value: URL Revision Author Date Id

Cosmetic

NOTOK

62

1) ArgumentNotValid on the connection. 2) Why do we throw an SQL exception? Usually we catch non-Netarkivet exceptions and throw Netarkivet exceptions instead. 3) Correct the javadoc. Add the connection and throws also ArgumentNotValid. And add the ArgumentNotValid to the method description.

Cosmetic

NOTOK

Lines

Description

Classification

Status

437

better naming. The name and the type should differ

Cosmetic

NOTOK

Lines

Description

Classification

Status

General

Missing svn:keywords property with value: URL Revision Author Date Id

Cosmetic

NOTOK

44

javadoc

Cosmetic

NOTOK

Lines

Description

Classification

Status

General

There are many retrieval methods. Consider inlining some of them.

Cosmetic

NOTOK

General

redundant logging when IOFailure is thrown at the same time: log.warn(msg); throw new IOFailure(msg, e); Suggest removing the log.warn(msg) statement

Cosmetic

NOTOK

General

Why this instead of just Date now = new Date(); Date now = new Date(Calendar.getInstance().getTimeInMillis());

Cosmetic

NOTOK

72

Describe that the settings are used for initialising the database

Cosmetic

NOTOK

104-105

Strange way for to check for existence or better comment. Why not instead: if (!repIds.contains(rep.getId)) { insert in database } else { repIds.remove(rep.getId()); log.info("");

Cosmetic

NOTOK

119-123

I wonder whether this shouldn't be fatal. Consider throwing an IllegalState in this case.

Minor

NOTOK

127, 133

Consider using the "in" instead of "within"

Cosmetic

NOTOK

132-133

IOFailure is not the correct exception here. It should be IllegalState instead.

Cosmetic

NOTOK

184, 219

Need more info in msg. Write instead: "Cannot add the following replica to the database: " + rep.toString()

NA

NOTOK

228

Change to "The fields for this new entry are set .."

Cosmetic

NOTOK

230

"each replica_id" is confusing. Consider changing from "each replica_id" to "The replica_id for the given replica"

Cosmetic

NOTOK

252

What does this mean?

Cosmetic

NOTOK

295-296

Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method)

Cosmetic

NOTOK

310-311

Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method)

Cosmetic

NOTOK

320

typo: "no such file_if" => "no such file_id"

Cosmetic

NOTOK

329

Wrong comment for the statements below

Cosmetic

NOTOK

359

typo: "retrieving thie" => retrieving the"

Cosmetic

NOTOK

401

Cut and paste. Change to "sql for retrieving the replicafileinfo_guid for the given fileid"

Cosmetic

NOTOK

425

Change "replica status" => "replica type"

Cosmetic

NOTOK

434

typo: "The if for the file" => "The id for the file"

Cosmetic

NOTOK

457-458

Change to " The SQL statement to retrieve the replica_id for the entry with the given replicafileinfo_guid ..."

Cosmetic

NOTOK

497-498, 502, 507

There is no given entry here!

Cosmetic

NOTOK

705

What does this mean? This is seen in other lines as well

Cosmetic

NOTOK

821, 836

This is the wrong exception to throw here. Use IllegalState instead

Cosmetic

NOTOK

849-850, 859-867

Instead of these findColumn statements, you could change the SELECT statement: SELECT replicafileinfo_guid, replica_id, file_id, segment_id, checksum, upload_status, filelist_status, filelist_checkdatetime, checksum_checkdatetime FROM replicafileinfo WHERE replicafileinfo_guid = ?" and then new ReplicaFileInfo(res.getLong(1), res.getString(2), res.getLong(3), res.getLong(4), res.getString(5), res.getInt(6), res.getInt(7), res.getDate(8), res.getDate(9)));

Cosmetic

NOTOK

898

valid => non-empty

Cosmetic

NOTOK

916-917

Missing argument validation in public method

Cosmetic

NOTOK

1154

missing validation of argument replica

Cosmetic

NOTOK

1209

Missing validation of arg replica

Cosmetic

NOTOK

1216

Add this information to the javadoc

Cosmetic

NOTOK

1236

Missing validation of arg replica

Cosmetic

NOTOK

1244

Add this information to the javadoc

Cosmetic

NOTOK

1264

Missing argumen validation

Cosmetic

NOTOK

1286

Missing argumen validation

Cosmetic

NOTOK

1307

Missing argumen validation

Cosmetic

NOTOK

1327

Missing argumen validation

Cosmetic

NOTOK

1350

Missing argumen validation

Cosmetic

NOTOK

1352-1353

You are checking in the database up against FileListStatus.OK not MISSING. Change text to " for the replica which have the filelist_status set to OK"

Cosmetic

NOTOK

1374

missing validation of argument

Cosmetic

NOTOK

1397-1398

Consider sending a notification about this. This is critical, isn't it?

Minor

NOTOK

1420-1421

Missing argument validation

Cosmetic

NOTOK

1447-1448

Shouldn't we send a notification about this?

Minor

NOTOK

Lines

Description

Classification

Status

General

Missing svn:keywords property with value: URL Revision Author Date Id

Cosmetic

NOTOK

54

javadoc

Cosmetic

NOTOK

86

javadoc the connection + permissiondenied

Cosmetic

NOTOK

Lines

Description

Classification

Status

Lines

Description

Classification

Status

General

Missing svn:keywords property with value: URL Revision Author Date Id

Cosmetic

NOTOK

84

Javadoc + throw PermissionDenied and IOFailure

Cosmetic

NOTOK