22159
Comment:
|
22122
|
Deletions are marked like this. | Additions are marked like this. |
Line 38: | Line 38: |
|| 237 || That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics || Cosmetic || NOTOK || || 312 || spelling: live => alive || Cosmetic || NOTOK || |
|| 237 || That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics || Cosmetic || INVALID || || 312 || spelling: live => alive || Cosmetic || OK || |
Line 42: | Line 42: |
|| 125 || ID => identifiers || Cosmetic || NOTOK || || 348 || Add to logentry and to javadoc: Returning the first one || Cosmetic || NOTOK || || 378 || Add: Returning the first one || Cosmetic || NOTOK || || 877 || Implement TODO, or remove TODO || Cosmetic || NOTOK || || 1386 || Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) || Cosmetic || NOTOK || || 1788 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || NOTOK || || 1821 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || NOTOK || || 1989 || spelling: bitarche => bitarchive || Cosmetic || NOTOK || || 2061 || Missing validation of argument 'line' || Cosmetic || NOTOK || || 2079-2080 || Remove? || Cosmetic || NOTOK || || 2132 || Add null validation of 'date' argument || Cosmetic || NOTOK || || 2160 || Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! || Cosmetic || NOTOK || |
|| 125 || ID => identifiers || Cosmetic || OK || || 348 || Add to logentry and to javadoc: Returning the first one || Cosmetic || OK || || 378 || Add: Returning the first one || Cosmetic || OK || || 877 || Implement TODO, or remove TODO || Cosmetic || OK || || 1386 || Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) || Cosmetic || OK || || 1788 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK || || 1821 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK || || 1989 || spelling: bitarche => bitarchive || Cosmetic || OK || || 2061 || Missing validation of argument 'line' || Cosmetic || OK || || 2079-2080 || Remove? || Cosmetic || OK || || 2132 || Add null validation of 'date' argument || Cosmetic || OK || || 2160 || Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! || Cosmetic || OK || |
Review (NS-147): Archive Assignment B.2.2 complete
Author |
Jonas |
Moderator |
Jonas |
State |
Closed |
Objectives
Besides containing all the latest work on database archival, it also contains a lot of code style changes.
Summary
Follow up by JOLF
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) JOLF: 8 MD SVC: 2 MD
General comments:
Description |
Classification |
Status |
Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs: -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop |
Cosmetic |
OK |
Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository" |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1320
Lines |
Description |
Classification |
Status |
161 |
make "_" as a constant |
Cosmetic |
OK |
180 |
Spelling: Receving => Receiving |
Cosmetic |
OK |
300 |
Spelling: Recieving => Receiving |
Cosmetic |
OK |
338 |
Spelling: Receving => Receiving |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaFileInfo.java', revision 1283
Lines |
Description |
Classification |
Status |
72 |
Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers) |
Cosmetic |
OK |
164 |
rename method: getFileListCheckdatetime() |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1283
Lines |
Description |
Classification |
Status |
237 |
That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics |
Cosmetic |
INVALID |
312 |
spelling: live => alive |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaCacheDatabase.java', revision 1311
Lines |
Description |
Classification |
Status |
125 |
ID => identifiers |
Cosmetic |
OK |
348 |
Add to logentry and to javadoc: Returning the first one |
Cosmetic |
OK |
378 |
Add: Returning the first one |
Cosmetic |
OK |
877 |
Implement TODO, or remove TODO |
Cosmetic |
OK |
1386 |
Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) |
Cosmetic |
OK |
1788 |
This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc |
Cosmetic |
OK |
1821 |
This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc |
Cosmetic |
OK |
1989 |
spelling: bitarche => bitarchive |
Cosmetic |
OK |
2061 |
Missing validation of argument 'line' |
Cosmetic |
OK |
2079-2080 |
Remove? |
Cosmetic |
OK |
2132 |
Add null validation of 'date' argument |
Cosmetic |
OK |
2160 |
Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 1300
Lines |
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1295
Lines |
Description |
Classification |
Status |
537 |
Spelling: Recieved => Received |
Cosmetic |
NOTOK |
597 |
Spelling: Recieved => Received |
Cosmetic |
NOTOK |
657 |
Spelling: Recieved => Received |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/LinuxMachine.java', revision 1305
Lines |
Description |
Classification |
Status |
1316-1318 |
Explain, that the output of shutdown is extended to the same file as the output when starting the database. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/ReestablishAdminDatabase.java', revision 1311
Lines |
Description |
Classification |
Status |
General |
missing SVN:keywords |
Cosmetic |
NOTOK |
145 |
Make into local constant or include with dk.netarkivet.archive.Constants |
Cosmetic |
NOTOK |
165 |
spelling: occured => occurred |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/ScriptConstants.java', revision 1305
Lines |
Description |
Classification |
Status |
70 |
Please rename to OPTION_SECURITY_POLICY |
Cosmetic |
NOTOK |
72 |
Please rename to OPTION_SECURITY_POLICY_WIN |
Cosmetic |
NOTOK |
74 |
This is not an appropriate description. |
Cosmetic |
NOTOK |
81-82 |
Change to one line |
Cosmetic |
NOTOK |
84-85 |
Change to one line |
Cosmetic |
NOTOK |
118 |
change to ETC_PROFILE |
Cosmetic |
NOTOK |
125-128 |
Replace with calls to StringUtils.repeat |
Cosmetic |
NOTOK |
356, 369, 382, 398, 414 |
These checkNotNulls should be replaced with checkNotNullOrEmpty |
Cosmetic |
NOTOK |
429, 447-449 |
These checkNotNulls should be replaced with checkNotNullOrEmpty |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage.java', revision 1289
Lines |
Description |
Classification |
Status |
84 |
replace 'to' with 'ChannelID to'. |
Cosmetic |
NOTOK |
85-86 |
replace 'baAppId' with 'String baAppId' |
Cosmetic |
NOTOK |
88 |
replace 'originatingBatchMsgId' with 'String originatingBatchMsgId' |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/UpdateableAdminData.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
There are multiple arguments in this class named "arcfileName". Rename as "fileName" |
Cosmetic |
NOTOK |
General |
All references til "arcrepository" should maybe be just "repository" |
Cosmetic |
NOTOK |
106 |
spelling: "ddmin data" => "admin data" |
Cosmetic |
NOTOK |
109, 191, 195 |
This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1298
Lines |
Description |
Classification |
Status |
173 |
Removal initial \n |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/Machine.java', revision 1302
Lines |
Description |
Classification |
Status |
94, 103 |
rename "bpdbFileName" as "arcdatabaseFilename" |
Cosmetic |
NOTOK |
681 |
Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CDXOriginCrawlLogIterator.java', revision 1289
Lines |
Description |
Classification |
Status |
162 |
Spelling: matchin=> matching |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/WindowsMachine.java', revision 1302
Lines |
Description |
Classification |
Status |
33 |
Why has this been added, when it is not used? |
Cosmetic |
NOTOK |
46 |
rename 'e' as 'root' |
Cosmetic |
NOTOK |
868-869 |
This indentation looks quite odd. Insert line break. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1320
Lines |
Description |
Classification |
Status |
123 |
Maybe, you should rather write: map (filename -> checksum) |
Cosmetic |
NOTOK |
140 |
Consider adding 'synchronized' to this method |
Cosmetic |
NOTOK |
341-346 |
Test whether the file is readable |
Cosmetic |
NOTOK |
360 |
make as a constant |
Cosmetic |
NOTOK |
400 |
Spelling: occured => occurred |
Cosmetic |
NOTOK |
529-550, 553-574 |
Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead |
Cosmetic |
NOTOK |
605 |
spelling: occured => occurred |
Cosmetic |
NOTOK |
645-657 |
Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DatabaseAdmin.java', revision 1285
Lines |
Description |
Classification |
Status |
General |
Add missing svn:keywords |
Cosmetic |
NOTOK |
67 |
Add "synchronized" to the method to prevent synchronization problems |
Cosmetic |
NOTOK |
117 |
add: uniquely identifying the replica |
Cosmetic |
NOTOK |
141-142 |
Is this still an outstanding issue? Add FR to handle this issue about storestate validity |
Cosmetic |
NOTOK |
295-296 |
Make a more meaningful text for this exception |
Cosmetic |
NOTOK |
346-348 |
Can be replaced with just instance = null; |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CombiningMultiFileBasedCache.java', revision 1283
Lines |
Description |
Classification |
Status |
45-46 |
Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface |
NA |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/AdminFactory.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
add SVN:keywords |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1321
Lines |
Description |
Classification |
Status |
423 |
Add @throws PermissionDenied if the credentials for correcting files are wrong |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabaseBasedActiveBitPreservation.java', revision 1293
Lines |
Description |
Classification |
Status |
221 |
Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it? |
Cosmetic |
NOTOK |
436 |
Add: valid meaning not null and not empty |
Cosmetic |
NOTOK |
463 |
add: invalid meaning not null; Should we accept not empty filenames here? |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/RemoveAndGetFileMessage.java', revision 1283
Lines |
Description |
Classification |
Status |
45, 61, 67, 70 |
Rename as "filename" |
Cosmetic |
NOTOK |
138-140 |
Rename as getFilename() |
Cosmetic |
NOTOK |
182 |
Arcfile => File |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/WorkFiles.java', revision 1288
Lines |
Description |
Classification |
Status |
254 |
Add "." after "below here" |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1298
Lines |
Description |
Classification |
Status |
95 |
Wrong comment: A new instance is now created every time |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/CreateTestInstance.java', revision 1301
Lines |
Description |
Classification |
Status |
139 |
Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT |
Cosmetic |
NOTOK |
141 |
Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestServer.java', revision 1296
Lines |
Description |
Classification |
Status |
199 |
Remove line |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 1289
Lines |
Description |
Classification |
Status |
246-249 |
Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009) |
NA |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1294
Lines |
Description |
Classification |
Status |
General |
The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum()) |
Cosmetic |
NOTOK |
73 |
We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map |
Cosmetic |
NOTOK |
107 |
The localisation should probably take place in the ReplicaStoreState class. This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state" |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/FileBasedCache.java', revision 1289
Lines |
Description |
Classification |
Status |
51 |
Spelling: The type of cache |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileListJob.java', revision 1289
Lines |
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/webinterface/BitpreserveFileState.java', revision 1290
Lines |
Description |
Classification |
Status |
86 |
Consider adding a log.warn message at this point |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbySpecifics.java', revision 1285
Lines |
Description |
Classification |
Status |
32 |
Add to javadoc, why this class is so short! |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/FileListStatus.java', revision 1276
Lines |
Description |
Classification |
Status |
30 |
=> This is used by .. |
Cosmetic |
NOTOK |
43-44 |
certain => specific |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Consider adding validation for the visit() methods in this class |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1283
Lines |
Description |
Classification |
Status |
962 |
Remove TODO; as we don't intend to implement this method |
Cosmetic |
NOTOK |
975 |
Remove TODO; as we don't intend to implement this method |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/deploy/Constants.java', revision 1304
Lines |
Description |
Classification |
Status |
168, 171, 181 |
HETRIX => HERITRIX |
Cosmetic |
NOTOK |
325 |
Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more. |
Cosmetic |
NOTOK |
518 |
Archive database ... |
Minor |
NOTOK |
577, 590 |
Be more strict: Disallow empty strings as valid scopes |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1298
Lines |
Description |
Classification |
Status |
241 |
Is this still true? If not, remove line |
Cosmetic |
NOTOK |
255 |
Change to "Failed to handle GetAllFilenamesMessage: " ... |
Cosmetic |
NOTOK |
276 |
Change to "Failed to handle GetAllChecksumsMessage: " ... |
Cosmetic |
NOTOK |
292 |
Spelling: Recieved => Received |
Cosmetic |
NOTOK |
319 |
Is this still true? If not remove TODO line |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/RawMetadataCache.java', revision 1310
Lines |
Description |
Classification |
Status |
180 |
Add: Currently does nothing |
Cosmetic |
NOTOK |
185-191, 205 |
throws IOFailure |
Cosmetic |
NOTOK |
206-210 |
delete? |
Cosmetic |
NOTOK |
216 |
Add: Currently does nothing |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbyServerSpecifics.java', revision 1283
Lines |
Description |
Classification |
Status |
44-49 |
Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabasePreservationState.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
missing license header |
Cosmetic |
NOTOK |
General |
Replace all(?) instances of "bitarchive" with "replica" in this class |
Cosmetic |
NOTOK |
General |
Consider adding a featurerequest for some of TODOs |
Cosmetic |
NOTOK |
General |
Consider replacing == with equals, if not certain that they have equal semantics |
Cosmetic |
NOTOK |
42 |
rename "file" as "filename" |
Cosmetic |
NOTOK |
92-93 |
Is this correct behaviour and if yes, document it in the javadoc. |
Cosmetic |
NOTOK |
98 |
If this is for presentation purposes only, move it to the archive.webinterface package |
Cosmetic |
NOTOK |
134-136 |
Add to javadoc, that it always returns true |
Cosmetic |
NOTOK |
158 |
Complete logmessage |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1298
Lines |
Description |
Classification |
Status |
211-217 |
why make a variable which is only used once? Make the string in the creating of the exception. |
Cosmetic |
NOTOK |
396 |
Refactor to be a method inside one of the Admin implementations |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DBConnect.java', revision 1300
Lines |
Description |
Classification |
Status |
114 |
Will this only work for an external database? If no update javadoc. |
Cosmetic |
NOTOK |
Comments on file 'trunk/webpages/BitPreservation/Bitpreservation-filestatus-checksum.jsp', revision 1288
Lines |
Description |
Classification |
Status |
126-129 |
Too long lines |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/Admin.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Add svn:keywords |
Cosmetic |
NOTOK |
45 |
add newline |
Cosmetic |
NOTOK |
61 |
add newline |
Cosmetic |
NOTOK |
64-65 |
Deprecate this method; as in the future it will not be possible to setChecksum |
Cosmetic |
NOTOK |
79 |
add newline |
Cosmetic |
NOTOK |
86 |
add newline |
Cosmetic |
NOTOK |
104 |
add newline |
Cosmetic |
NOTOK |
108 |
This is not decided in the interface, but in the actual implementation(s) |
Cosmetic |
NOTOK |
116 |
add newline |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ArchiveStoreState.java', revision 1283
Lines |
Description |
Classification |
Status |
74 |
throws ArgumentNotValid! |
Cosmetic |
NOTOK |
90 |
throws ArgumentNotValid |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/PreservationState.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Add svn:keywords |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveServer.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Consider adding validation for the visit methods in this class |
Cosmetic |
NOTOK |