= Review (NS-129): Changes caused by progress in Assignment B.2.2b. =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Closed ||
== Objectives ==
{{{
The DatabaseBasedActiveBitPreservation is not finished, and it is therefore not important the it works completely.
It has to be ensured that current functionality still works.
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
JOLF: 3 md
SVC: 0.5 md
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| Consider starting the all // comments with a capital letter. Currently this is not done consistently. || Cosmetic || REJECT ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 112 || Consider synchronizing the method || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/complete_settings.xml', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 439-440 || The name and the type should not be the same. || Cosmetic || REJECT ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 130 || Consider removing validation. Validation is only required for public methods || Cosmetic || OK ||
|| 298 || Change to: if (referenceCheckSum.isEmpty()) || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbyServerSpecifics.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Missing svn:keywords property with value: URL Revision Author Date Id || Cosmetic || OK ||
|| 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 || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/settings.xml', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 437 || better naming. The name and the type should differ || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbySpecifics.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Missing svn:keywords property with value: URL Revision Author Date Id || Cosmetic || OK ||
|| 44 || javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/ReplicaCacheDatabase.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || There are many retrieval methods. Consider inlining some of them. || Cosmetic ||POSTPONE ||
|| 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 || OK ||
|| General || Why this instead of just Date now = new Date();  Date now = new Date(Calendar.getInstance().getTimeInMillis()); || Cosmetic || OK ||
|| 72 || Describe that the settings are used for initialising the database || Cosmetic || OK ||
|| 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 || OK ||
|| 119-123 || I wonder whether this shouldn't be fatal. Consider throwing an IllegalState in this case. || Minor || OK ||
|| 127, 133 || Consider using the "in" instead of "within" || Cosmetic || OK ||
|| 132-133 || IOFailure is not the correct exception here. It should be IllegalState instead. || Cosmetic || OK ||
|| 184, 219 || Need more info in msg. Write instead: "Cannot add the following replica to the database: " + rep.toString() || NA || OK ||
|| 228 || Change to "The fields for this new entry are set .." || Cosmetic || OK ||
|| 230 || "each replica_id" is confusing. Consider changing from "each replica_id" to "The replica_id for the given replica" || Cosmetic || OK ||
|| 252 || What does this mean? || Cosmetic || OK ||
|| 295-296 || Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method) || Cosmetic || OK ||
|| 310-311 || Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method) || Cosmetic || OK ||
|| 320 || typo: "no such file_if" => "no such file_id" || Cosmetic || OK ||
|| 329 || Wrong comment for the statements below || Cosmetic || OK ||
|| 359 || typo: "retrieving thie" => retrieving the" || Cosmetic || OK ||
|| 401 || Cut and paste. Change to "sql for retrieving the replicafileinfo_guid for the given fileid" || Cosmetic || OK ||
|| 425 || Change "replica status" => "replica type" || Cosmetic || OK ||
|| 434 || typo: "The if for the file" => "The id for the file" || Cosmetic || OK ||
|| 457-458 || Change to " The SQL statement to retrieve the replica_id for the entry with the given replicafileinfo_guid ..." || Cosmetic || OK ||
|| 497-498, 502, 507 || There is no given entry here! || Cosmetic || OK ||
|| 705 || What does this mean? This is seen in other lines as well || Cosmetic || OK ||
|| 821, 836 || This is the wrong exception to throw here. Use IllegalState instead || Cosmetic || OK ||
|| 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 || OK ||
|| 898 || valid => non-empty || Cosmetic || OK ||
|| 916-917 || Missing argument validation in public method || Cosmetic || OK ||
|| 1154 || missing validation of argument replica || Cosmetic || OK ||
|| 1209 || Missing validation of arg replica || Cosmetic || OK ||
|| 1216 || Add this information to the javadoc || Cosmetic || OK ||
|| 1236 || Missing validation of arg replica || Cosmetic || OK ||
|| 1244 || Add this information to the javadoc || Cosmetic || OK ||
|| 1264 || Missing argumen validation || Cosmetic || OK ||
|| 1286 || Missing argumen validation || Cosmetic || OK ||
|| 1307 || Missing argumen validation || Cosmetic || OK ||
|| 1327 || Missing argumen validation || Cosmetic || OK ||
|| 1350 || Missing argumen validation || Cosmetic || OK ||
|| 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 || OK ||
|| 1374 || missing validation of argument || Cosmetic || OK ||
|| 1397-1398 || Consider sending a notification about this. This is critical, isn't it? || Minor || OK ||
|| 1420-1421 || Missing argument validation || Cosmetic || OK ||
|| 1447-1448 || Shouldn't we send a notification about this? || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DBSpecifics.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Missing svn:keywords property with value: URL Revision Author Date Id || Cosmetic || OK ||
|| 54 || javadoc || Cosmetic || OK ||
|| 86 || javadoc the connection + permissiondenied || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbyEmbeddedSpecifics.java', revision 1179 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Missing svn:keywords property with value: URL Revision Author Date Id || Cosmetic || OK ||
|| 84 || Javadoc + throw PermissionDenied and IOFailure || Cosmetic || OK ||