17632
Comment:
|
← Revision 4 as of 2010-08-16 10:24:30 ⇥
17602
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 45: | Line 45: |
|| 314 || Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? || Cosmetic ||POSTPONED|| | || 314 || Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? || Cosmetic ||POSTPONED || |
Line 52: | Line 52: |
|| 149, 152 || Add constants for ".class", and ".jar", and maybe "." || Cosmetic || NOTOK || || 164 || Spell CWD out: Current working directory || Cosmetic || NOTOK || || 202, 204 || public or private variables? || Cosmetic || NOTOK || || 205-206 || What is meant by this comment? Add TODO: Use the HelpFormatter class to print out Usage information || Cosmetic || NOTOK || || 215, 217, 220, 224, 227, 234, 237 || Consider replacing by constants || Cosmetic || NOTOK || || 220-221 || Indentation || Cosmetic || NOTOK || || 222 || Odd place for an empty line. || Cosmetic || NOTOK || || 241 || Javadoc. public/private/protected not defined for this method? || Cosmetic || NOTOK || || 252 || Javadoc. public/private/protected not defined for this method? || Cosmetic || NOTOK || || 266-267 || Two empty lines in a row. || Cosmetic || NOTOK || || 268-269 || Shouldn't variables be placed next to each other, and not randomly in the middle of the methods? || Cosmetic || NOTOK || || 340 || change "," to a constant. || Cosmetic || NOTOK || || 355 || Make into bug or feature request (if not already) and refer to this. Or implement || Cosmetic || NOTOK || || 505-506 || Two empty lines in a row || Cosmetic || NOTOK || |
|| 149, 152 || Add constants for ".class", and ".jar", and maybe "." || Cosmetic || OK || || 164 || Spell CWD out: Current working directory || Cosmetic || OK || || 202, 204 || public or private variables? || Cosmetic ||POSTPONE || || 205-206 || What is meant by this comment? Add TODO: Use the HelpFormatter class to print out Usage information || Cosmetic || OK || || 215, 217, 220, 224, 227, 234, 237 || Consider replacing by constants || Cosmetic || OK || || 220-221 || Indentation || Cosmetic || OK || || 222 || Odd place for an empty line. || Cosmetic || OK || || 241 || Javadoc. public/private/protected not defined for this method? || Cosmetic || POSTPONE || || 252 || Javadoc. public/private/protected not defined for this method? || Cosmetic || POSTPONE || || 266-267 || Two empty lines in a row. || Cosmetic || OK || || 268-269 || Shouldn't variables be placed next to each other, and not randomly in the middle of the methods? || Cosmetic || OK || || 340 || change "," to a constant. || Cosmetic || OK || || 355 || Make into bug or feature request (if not already) and refer to this. Or implement || Cosmetic || OK || || 505-506 || Two empty lines in a row || Cosmetic || OK || |
Line 75: | Line 75: |
|| 51 || ".*" should perhaps be a constants (static final) variable. || Cosmetic || NOTOK || || 109 || ".*" as a constant (static final) variable (same constant as previous) || Cosmetic || NOTOK || || 138 || Make an OR_SEPARATOR constant for "|" || Cosmetic || NOTOK || || 164, 167 || It does not need to be ARC-files we're processing || Cosmetic || NOTOK || || 174-175, 177-178 || It does not need to be ARC-files we're processing || Cosmetic || NOTOK || || 206, 222 || Log the case, if maxExceptions have been reached || Cosmetic || NOTOK || || 235 || Log the case, if maxExceptions have been reached || Cosmetic || NOTOK || || 259 || Add a TODO, that this should/could be a setting || Cosmetic || NOTOK || || 330 || Replace -1 with UNKNOWN_OFFSET || Cosmetic || NOTOK || || 393-398 || Javadoc this method. || Cosmetic || NOTOK || |
|| 51 || ".*" should perhaps be a constants (static final) variable. || Cosmetic || OK || || 109 || ".*" as a constant (static final) variable (same constant as previous) || Cosmetic || OK || || 138 || Make an OR_SEPARATOR constant for "|" || Cosmetic || POSTPONE || || 164, 167 || It does not need to be ARC-files we're processing || Cosmetic || OK || || 174-175, 177-178 || It does not need to be ARC-files we're processing || Cosmetic || OK || || 206, 222 || Log the case, if maxExceptions have been reached || Cosmetic || OK || || 235 || Log the case, if maxExceptions have been reached || Cosmetic || OK || || 259 || Add a TODO, that this should/could be a setting || Cosmetic || OK || || 330 || Replace -1 with UNKNOWN_OFFSET || Cosmetic || OK || || 393-398 || Javadoc this method. || Cosmetic || OK || |
Line 87: | Line 87: |
|| General || Rename BatchFilter as ARCBatchFilter. || Cosmetic || NOTOK || || 117 || Make comment about 'mimetypeIsOk' checks for ArgumentNotValid, or check the mimetype for ArgumentNotValid. || Cosmetic || NOTOK || |
|| General || Rename BatchFilter as ARCBatchFilter. || Cosmetic || OK || || 117 || Make comment about 'mimetypeIsOk' checks for ArgumentNotValid, or check the mimetype for ArgumentNotValid. || Cosmetic || OK || |
Line 91: | Line 91: |
|| 64 || Add local variables for the 'null', and '0' constants: String name = null; int offset = 0; || Cosmetic || NOTOK || | || 64 || Add local variables for the 'null', and '0' constants: String name = null; int offset = 0; || Cosmetic || POSTPONE || |
Line 94: | Line 94: |
|| 43 || javadoc || Cosmetic || NOTOK || || 49 || Replace "used for batching" by "processed by the batch job" || Cosmetic || NOTOK || || 55-58 || This would be the same as: ArgumentNotValid.checkNotNull(incomingFile[i], "Null element at index " + i + " in file lise for batch."); Thus using the check function to check for NotNull. || Cosmetic || NOTOK || || 84 || Should we add the initialization exception to the list of exceptions for job? Add as TODO || Cosmetic || NOTOK || || 90 || Should we add this finish exception to the list of exceptions for job? Add as TODO || Cosmetic || NOTOK || || 108-109 || Should we add this exception thrown during processing to the list of exceptions for job? Add as TODO || Cosmetic || NOTOK || |
|| 43 || javadoc || Cosmetic || OK || || 49 || Replace "used for batching" by "processed by the batch job" || Cosmetic || OK || || 55-58 || This would be the same as: ArgumentNotValid.checkNotNull(incomingFile[i], "Null element at index " + i + " in file lise for batch."); Thus using the check function to check for NotNull. || Cosmetic || OK || || 84 || Should we add the initialization exception to the list of exceptions for job? Add as TODO || Cosmetic || OK || || 90 || Should we add this finish exception to the list of exceptions for job? Add as TODO || Cosmetic || OK || || 108-109 || Should we add this exception thrown during processing to the list of exceptions for job? Add as TODO || Cosmetic || OK || |
Line 102: | Line 102: |
|| 48 || language: "resent" i.e. forwarded || Cosmetic || NOTOK || || 74 || Rename con as jmscon || Cosmetic || NOTOK || || 120 || Consider checking that inbMsg is not null || Cosmetic || NOTOK || || 144 || Consider checking that beMsg is not null || Cosmetic || NOTOK || || 176 || Consider checking that hbMsg is not null || Cosmetic || NOTOK || || 192 || 'o' is not a very good variable name og an observable object. Use 'obs' or equivilent; || Cosmetic || NOTOK || || 202, 206, 212 || Add respectively "an", "an", and "a" after "Received" || Cosmetic || NOTOK || |
|| 48 || language: "resent" i.e. forwarded || Cosmetic || POSTPONE || || 74 || Rename con as jmscon || Cosmetic || POSTPONE || || 120 || Consider checking that inbMsg is not null || Cosmetic || POSTPONE || || 144 || Consider checking that beMsg is not null || Cosmetic || POSTPONE || || 176 || Consider checking that hbMsg is not null || Cosmetic || POSTPONE || || 192 || 'o' is not a very good variable name og an observable object. Use 'obs' or equivilent; || Cosmetic || POSTPONE || || 202, 206, 212 || Add respectively "an", "an", and "a" after "Received" || Cosmetic || POSTPONE || |
Line 111: | Line 111: |
|| 45 || "to identifier for" => "to identify" || Cosmetic || NOTOK || || 48 || Rename "BA_ApplicationId" as "BitarchiveApplicationId" || Cosmetic || NOTOK || || 59-61 || three empty lines in a row. Delete two of them. || Cosmetic || NOTOK || || 73 || rename to BitarchiveApplicationId || Cosmetic || NOTOK || || 76 || "he remote file" => "The remote file" || Cosmetic || NOTOK || || 177 || Add ArgumentNotValid check || Cosmetic || NOTOK || |
|| 45 || "to identifier for" => "to identify" || Cosmetic || POSTPONE || || 48 || Rename "BA_ApplicationId" as "BitarchiveApplicationId" || Cosmetic || POSTPONE || || 59-61 || three empty lines in a row. Delete two of them. || Cosmetic || POSTPONE || || 73 || rename to BitarchiveApplicationId || Cosmetic || POSTPONE || || 76 || "he remote file" => "The remote file" || Cosmetic || POSTPONE || || 177 || Add ArgumentNotValid check || Cosmetic || POSTPONE || |
Line 119: | Line 119: |
|| 116 || Add a JAVA_PACKAGE_SEPARATOR constant for "." Add a DIRECTORY_SEPARATOR constant for "/" || Cosmetic || NOTOK || || 119 || Replace "No data loaded!!!!!" with "No data loaded for class with name '" + className + "'." || Cosmetic || NOTOK || || 157 || Check argument os || Cosmetic || NOTOK || || 170 || 'Cannout' = spelling error. || Cosmetic || NOTOK || || 192 || "Finish up" => "Finish" || Cosmetic || NOTOK || |
|| 116 || Add a JAVA_PACKAGE_SEPARATOR constant for "." Add a DIRECTORY_SEPARATOR constant for "/" || Cosmetic || OK || || 119 || Replace "No data loaded!!!!!" with "No data loaded for class with name '" + className + "'." || Cosmetic || OK || || 157 || Check argument os || Cosmetic || OK || || 170 || 'Cannout' = spelling error. || Cosmetic || OK || || 192 || "Finish up" => "Finish" || Cosmetic || OK || |
Line 126: | Line 126: |
|| 35 || Delete line || Cosmetic || NOTOK || || 58-59 || ".., that run this batchJob" => ".., that ran this batch job" || Cosmetic || NOTOK || || 66-70 || Check for ArgumentNotValid. || Cosmetic || NOTOK || || 90 || Make "ALL_BITARCHIVE_APPS" a constant || Cosmetic || NOTOK || || 160 || ".. to copy into '" => ".. to copy into target file '" || Cosmetic || NOTOK || || 161 || "batch job on" => "batch job run on " || Cosmetic || NOTOK || || 168 || "so this" => so this method || Cosmetic || NOTOK || || 187 || the variable 'stream' cannot be printed out. Just write "append to output stream". || Cosmetic || NOTOK || || 188 || "batch job on" => batch job run on bitarchive application || Cosmetic || NOTOK || |
|| 35 || Delete line || Cosmetic || POSTPONE || || 58-59 || ".., that run this batchJob" => ".., that ran this batch job" || Cosmetic || POSTPONE || || 66-70 || Check for ArgumentNotValid. || Cosmetic || POSTPONE || || 90 || Make "ALL_BITARCHIVE_APPS" a constant || Cosmetic || POSTPONE || || 160 || ".. to copy into '" => ".. to copy into target file '" || Cosmetic || POSTPONE || || 161 || "batch job on" => "batch job run on " || Cosmetic || POSTPONE || || 168 || "so this" => so this method || Cosmetic || POSTPONE || || 187 || the variable 'stream' cannot be printed out. Just write "append to output stream". || Cosmetic || POSTPONE || || 188 || "batch job on" => batch job run on bitarchive application || Cosmetic || POSTPONE || |
Line 137: | Line 137: |
|| 57 || Check args arcfile and cdxstream || Cosmetic || NOTOK || || 59 || Remove superfluous whitespace || Cosmetic || NOTOK || || 75 || Remove superfluous whitespace || Cosmetic || NOTOK || || 101-105 || replace with: ArgumentNotValid.checkTrue(arcFileDirectory.isDirectory() && arcFileDirectory.canRead(), "..."); || Cosmetic || NOTOK || || 106-110 || replace with: ArgumentNotValid.checkTrue(cdxFileDirectory.isDirectory() && cdxFileDirectory.canRead(), "..."); || Cosmetic || NOTOK || || 140 || could be written if (!exceptions.isEmpty()) || Cosmetic || NOTOK || |
|| 57 || Check args arcfile and cdxstream || Cosmetic ||POSTPONE || || 59 || Remove superfluous whitespace || Cosmetic || POSTPONE || || 75 || Remove superfluous whitespace || Cosmetic || POSTPONE || || 101-105 || replace with: ArgumentNotValid.checkTrue(arcFileDirectory.isDirectory() && arcFileDirectory.canRead(), "..."); || Cosmetic || POSTPONE || || 106-110 || replace with: ArgumentNotValid.checkTrue(cdxFileDirectory.isDirectory() && cdxFileDirectory.canRead(), "..."); || Cosmetic || POSTPONE || || 140 || could be written if (!exceptions.isEmpty()) || Cosmetic || POSTPONE || |
Line 145: | Line 145: |
|| 33-36, 71-73 || Only three of the four new values are included. Add the fourth || Cosmetic || NOTOK || | || 33-36, 71-73 || Only three of the four new values are included. Add the fourth || Cosmetic || OK || |
Line 148: | Line 148: |
|| 106 || ArgumentNotValid.checkNotNegative on 'index' ? || Cosmetic || NOTOK || || 112 || don't use 'arcfile' before it is checked for 'ArgumentNotValid' on the following line. || Cosmetic || NOTOK || || 136-140 || Remove uncommented code || Cosmetic || NOTOK || || 183 || check for ArgumentNotValid before use of argument-variables. || Cosmetic || NOTOK || || 261-264 || Identical messages, use a string. || Cosmetic || NOTOK || || 318 || Move log after argument validation || Cosmetic || NOTOK || || 338 || Should this not be synchronized to avoid multiple callers beginning the construction of a new Bitarchive object || Cosmetic || NOTOK || |
|| 106 || ArgumentNotValid.checkNotNegative on 'index' ? || Cosmetic ||POSTPONE || || 112 || don't use 'arcfile' before it is checked for 'ArgumentNotValid' on the following line. || Cosmetic || POSTPONE || || 136-140 || Remove uncommented code || Cosmetic || POSTPONE || || 183 || check for ArgumentNotValid before use of argument-variables. || Cosmetic || POSTPONE || || 261-264 || Identical messages, use a string. || Cosmetic || POSTPONE || || 318 || Move log after argument validation || Cosmetic || POSTPONE || || 338 || Should this not be synchronized to avoid multiple callers beginning the construction of a new Bitarchive object || Cosmetic || POSTPONE || |
Line 157: | Line 157: |
|| 52, 56, 58-60 || Javadoc. || Cosmetic || NOTOK || || 76 || Add ArgumentNotValid check for file, and file existence || Cosmetic || NOTOK || || 90 || make ArgumentNotValid checks for arcfile (Not Null Not empty), and index (not negative) || Cosmetic || NOTOK || || 123 || Note that this argument is not used in this implementation || Cosmetic || NOTOK || || 128 || check for Argument not valid? || Cosmetic || NOTOK || || 133 || Only need to write "batch" once || Cosmetic || NOTOK || || 143 || Check for ArgumentNotValid. || Cosmetic || NOTOK || || 147-148 || Remove uncommented code || Cosmetic || NOTOK || || 163 || Write "batch job" instead of just "batch" || Cosmetic || NOTOK || || 188-190 || Throw an NotImplementedException || Cosmetic || NOTOK || || 199-200 || Throw an NotImplementedException || Cosmetic || NOTOK || || 213-214 || check for ArgumentNotValid. || Cosmetic || NOTOK || || 215 || note in the javadoc, that these variables are not used || Cosmetic || NOTOK || |
|| 52, 56, 58-60 || Javadoc. || Cosmetic || OK || || 76 || Add ArgumentNotValid check for file, and file existence || Cosmetic || OK || || 90 || make ArgumentNotValid checks for arcfile (Not Null Not empty), and index (not negative) || Cosmetic || OK || || 123 || Note that this argument is not used in this implementation || Cosmetic || OK || || 128 || check for Argument not valid? || Cosmetic || OK || || 133 || Only need to write "batch" once || Cosmetic || OK || || 143 || Check for ArgumentNotValid. || Cosmetic || OK || || 147-148 || Remove uncommented code || Cosmetic || OK || || 163 || Write "batch job" instead of just "batch" || Cosmetic || OK || || 188-190 || Throw an NotImplementedException || Cosmetic || OK || || 199-200 || Throw an NotImplementedException || Cosmetic || OK || || 213-214 || check for ArgumentNotValid. || Cosmetic || OK || || 215 || note in the javadoc, that these variables are not used || Cosmetic || OK || |
Line 172: | Line 172: |
|| 52 || Javadoc. || Cosmetic || NOTOK || || 164, 343 || use instead files.toArray(new File[files.size]) || Cosmetic || NOTOK || || 164 || Too long line || Cosmetic || NOTOK || || 262 || Add meaning that the given File object exists, is a directory, and is writable by this process. || Cosmetic || NOTOK || || 298-305 || The 'j' is not used explicit -> change to "for (File file : filesHere)" and use 'file' instead of 'filesHere[j]'. || Cosmetic || NOTOK || || 300 || replace 'filesHere[j]' with "filesHere[j].getAbsolutePath()"? || Cosmetic || NOTOK || || 319 || Validate regexp as Not null and Not empty String || Cosmetic || NOTOK || || 329-330 || Replace "Non-file" with "Not a regular File" || Cosmetic || NOTOK || || 364 || "Corrupt" => "Possibly corrupt" || Cosmetic || NOTOK || || 370 || Consider logging at trace || Cosmetic || NOTOK || || 388-395 || use for (File file : files) { ... } || Cosmetic || NOTOK || || 404-412 || use for (File file : files) { ... } || Cosmetic || NOTOK || || 421-429 || use for (File file : files) { ... } || Cosmetic || NOTOK || || 471-474 || Different messages. || Cosmetic || NOTOK || || 479-480 || Why use two line for this? || Cosmetic || NOTOK || |
|| 52 || Javadoc. || Cosmetic || OK || || 164, 343 || use instead files.toArray(new File[files.size]) || Cosmetic || OK || || 164 || Too long line || Cosmetic || OK || || 262 || Add meaning that the given File object exists, is a directory, and is writable by this process. || Cosmetic || OK || || 298-305 || The 'j' is not used explicit -> change to "for (File file : filesHere)" and use 'file' instead of 'filesHere[j]'. || Cosmetic || OK || || 300 || replace 'filesHere[j]' with "filesHere[j].getAbsolutePath()"? || Cosmetic || OK || || 319 || Validate regexp as Not null and Not empty String || Cosmetic || OK || || 329-330 || Replace "Non-file" with "Not a regular File" || Cosmetic || OK || || 364 || "Corrupt" => "Possibly corrupt" || Cosmetic || OK || || 370 || Consider logging at trace || Cosmetic || OK || || 388-395 || use for (File file : files) { ... } || Cosmetic || OK || || 404-412 || use for (File file : files) { ... } || Cosmetic || OK || || 421-429 || use for (File file : files) { ... } || Cosmetic || OK || || 471-474 || Different messages. || Cosmetic ||POSTPONE || || 479-480 || Why use two line for this? || Cosmetic || OK || |
Line 189: | Line 189: |
|| General || Should this also read the MINSPACELEFT setting used by the Bitarchives? Add as TODO || Cosmetic || NOTOK || || 71-72 || Javadoc. || Cosmetic || NOTOK || || 79 || Add a log.info message, that the DEFAULT_DIR_NAME is included as one of the fileDirs || Cosmetic || NOTOK || || 105 || throws also IllegalState. Either add IllegalState, or remove the others. || Cosmetic || NOTOK || || 175 || Mention that the location argument is unused in this implementation || Cosmetic || NOTOK || || 225 || Perhaps also check for '!filesInDir.isEmpty' to aviod making a unnecessary call to the 'files.addAll' method. || Cosmetic || NOTOK || || 248 || Remove uncommented code || Cosmetic || NOTOK || || 260-262 || Throw NotImplementedException, and add note about this to javadoc || Cosmetic || NOTOK || || 271-272 || Throw NotImplementedException and add note about this to javadoc || Cosmetic || NOTOK || || 279-280 || Add that bitarchiveName is unused in this implementation of ArcRepositoryClient And is therefore not checked || Cosmetic || NOTOK || |
|| General || Should this also read the MINSPACELEFT setting used by the Bitarchives? Add as TODO || Cosmetic || POSTPONE || || 71-72 || Javadoc. || Cosmetic || POSTPONE || || 79 || Add a log.info message, that the DEFAULT_DIR_NAME is included as one of the fileDirs || Cosmetic || POSTPONE || || 105 || throws also IllegalState. Either add IllegalState, or remove the others. || Cosmetic || POSTPONE || || 175 || Mention that the location argument is unused in this implementation || Cosmetic || POSTPONE || || 225 || Perhaps also check for '!filesInDir.isEmpty' to aviod making a unnecessary call to the 'files.addAll' method. || Cosmetic ||POSTPONE || || 248 || Remove uncommented code || Cosmetic || POSTPONE || || 260-262 || Throw NotImplementedException, and add note about this to javadoc || Cosmetic ||POSTPONE || || 271-272 || Throw NotImplementedException and add note about this to javadoc || Cosmetic || POSTPONE || || 279-280 || Add that bitarchiveName is unused in this implementation of ArcRepositoryClient And is therefore not checked || Cosmetic || POSTPONE || |
Line 201: | Line 201: |
|| 45 || remove empty line || Cosmetic || NOTOK || || 97 || "if file" => if file was || Cosmetic || NOTOK || || 135-140 || This is catched by the outer try-catch, and handled exactly the same way. Remove try-catch || Minor || NOTOK || || 139 || Add comment: "This aborts the processing of this ARC-file" || Cosmetic || NOTOK || || 160 || log here: Unexpected exception caught. The processing of the rest of the arcrecords aborted || Cosmetic || NOTOK || || 193-195 || Javadoc this, and argument not valid (or describe why not). || Cosmetic || NOTOK || || 206, 209 || Are we supposed to write the 'throws ArgumentNotValid' for all the methods, which makes the ArgumentNotValid check? NO Because then it is needed in a lot of methods. || Cosmetic || NOTOK || || 208 || Check 'ArgumentNotValid' on 'arcfile' (checkNotNull) and 'index' (checkNotNegative). || Cosmetic || NOTOK || || 236-238 || Javadoc || Cosmetic || NOTOK || |
|| 45 || remove empty line || Cosmetic || POSTPONE || || 97 || "if file" => if file was || Cosmetic || POSTPONE || || 135-140 || This is catched by the outer try-catch, and handled exactly the same way. Remove try-catch || Minor || OK || || 139 || Add comment: "This aborts the processing of this ARC-file" || Cosmetic ||REJECTED || || 160 || log here: Unexpected exception caught. The processing of the rest of the arcrecords aborted || Cosmetic || POSTPONE || || 193-195 || Javadoc this, and argument not valid (or describe why not). || Cosmetic || OK || || 206, 209 || Are we supposed to write the 'throws ArgumentNotValid' for all the methods, which makes the ArgumentNotValid check? NO Because then it is needed in a lot of methods. || Cosmetic || OK || || 208 || Check 'ArgumentNotValid' on 'arcfile' (checkNotNull) and 'index' (checkNotNegative). || Cosmetic || REJECTED || || 236-238 || Javadoc || Cosmetic || OK || |
Line 212: | Line 212: |
|| 42 || Javadoc. || Cosmetic || NOTOK || | || 42 || Javadoc. || Cosmetic || POSTPONE || |
Review (NS-27): Review of changes to batch framework
Author |
Søren |
Moderator |
Søren |
State |
Closed |
Objectives
To review the current code in the NetarchiveSuite batch framework, including the tool RunBatch. The review includes a review of all the lines in these files: /trunk/src/dk/netarkivet/common/utils/batch/LoadableJarBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/FileBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/BatchLocalFiles.java /trunk/src/dk/netarkivet/common/utils/batch/ByteClassLoader.java /trunk/src/dk/netarkivet/common/utils/batch/BatchFilter.java /trunk/src/dk/netarkivet/common/utils/arc/ARCBatchJob.java /trunk/src/dk/netarkivet/archive/tools/RunBatch.java /trunk/src/dk/netarkivet/common/distribute/arcrepository/BatchStatus.java trunk/src/dk/netarkivet/common/utils/ExtractCDX These files are only to be reviewed in the parts defined by the given linenumbers: trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin trunk/src/dk/netarkivet/archive/arcrepository/JMSArcRepositoryClient trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage trunk/build.xml
Summary
Review with JOLF completed. Follow up by SVC
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 544
Lines |
Description |
Classification |
Status |
125 |
ArgumentNotValid.checkNotNullOrEmpty(appID); ? |
Cosmetic |
OK |
131 |
"from" => "from bitarchive" |
Cosmetic |
OK |
221-229 |
Does not check ArgumentNotValid on: 'noOfFilesProcessed', 'filesFailed', 'remoteFile', 'errMsg', 'exceptions'. |
Cosmetic |
OK |
305, 314 |
Perhaps consider to change to 'private' with 'get' and 'set' methods instead of public local variables. |
Cosmetic |
POSTPONED |
314 |
Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? |
Cosmetic |
POSTPONED |
354-355 |
Log this |
Cosmetic |
OK |
370 |
Check for ArgumentNotValid? |
Cosmetic |
REJECTED |
412-421 |
'found' only used once. Thus change to if(!missingRespondents.remove(bitarchiveID)) { ... } OR write comment about 'found' |
Cosmetic |
OK |
522 |
check for ArgumentNotValid? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 737
Lines |
Description |
Classification |
Status |
149, 152 |
Add constants for ".class", and ".jar", and maybe "." |
Cosmetic |
OK |
164 |
Spell CWD out: Current working directory |
Cosmetic |
OK |
202, 204 |
public or private variables? |
Cosmetic |
POSTPONE |
205-206 |
What is meant by this comment? Add TODO: Use the HelpFormatter class to print out Usage information |
Cosmetic |
OK |
215, 217, 220, 224, 227, 234, 237 |
Consider replacing by constants |
Cosmetic |
OK |
220-221 |
Indentation |
Cosmetic |
OK |
222 |
Odd place for an empty line. |
Cosmetic |
OK |
241 |
Javadoc. public/private/protected not defined for this method? |
Cosmetic |
POSTPONE |
252 |
Javadoc. public/private/protected not defined for this method? |
Cosmetic |
POSTPONE |
266-267 |
Two empty lines in a row. |
Cosmetic |
OK |
268-269 |
Shouldn't variables be placed next to each other, and not randomly in the middle of the methods? |
Cosmetic |
OK |
340 |
change "," to a constant. |
Cosmetic |
OK |
355 |
Make into bug or feature request (if not already) and refer to this. Or implement |
Cosmetic |
OK |
505-506 |
Two empty lines in a row |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 544
Lines |
Description |
Classification |
Status |
211-213 |
Identical messages -> make into string before use. |
Cosmetic |
OK |
268-271 |
ArgumentNotValid.checkTrue(file.isFile(), "...."); |
Cosmetic |
OK |
273 |
Better to use StringBuilder. |
Cosmetic |
OK |
387, 407 |
Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN |
Cosmetic |
OK |
412, 426 |
Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/FileBatchJob.java', revision 548
Lines |
Description |
Classification |
Status |
51 |
".*" should perhaps be a constants (static final) variable. |
Cosmetic |
OK |
109 |
".*" as a constant (static final) variable (same constant as previous) |
Cosmetic |
OK |
138 |
Make an OR_SEPARATOR constant for "|" |
Cosmetic |
POSTPONE |
164, 167 |
It does not need to be ARC-files we're processing |
Cosmetic |
OK |
174-175, 177-178 |
It does not need to be ARC-files we're processing |
Cosmetic |
OK |
206, 222 |
Log the case, if maxExceptions have been reached |
Cosmetic |
OK |
235 |
Log the case, if maxExceptions have been reached |
Cosmetic |
OK |
259 |
Add a TODO, that this should/could be a setting |
Cosmetic |
OK |
330 |
Replace -1 with UNKNOWN_OFFSET |
Cosmetic |
OK |
393-398 |
Javadoc this method. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchFilter.java', revision 548
Lines |
Description |
Classification |
Status |
General |
Rename BatchFilter as ARCBatchFilter. |
Cosmetic |
OK |
117 |
Make comment about 'mimetypeIsOk' checks for ArgumentNotValid, or check the mimetype for ArgumentNotValid. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/ByteClassLoader.java', revision 548
Lines |
Description |
Classification |
Status |
64 |
Add local variables for the 'null', and '0' constants: String name = null; int offset = 0; |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchLocalFiles.java', revision 548
Lines |
Description |
Classification |
Status |
43 |
javadoc |
Cosmetic |
OK |
49 |
Replace "used for batching" by "processed by the batch job" |
Cosmetic |
OK |
55-58 |
This would be the same as: ArgumentNotValid.checkNotNull(incomingFile[i], "Null element at index " + i + " in file lise for batch."); Thus using the check function to check for NotNull. |
Cosmetic |
OK |
84 |
Should we add the initialization exception to the list of exceptions for job? Add as TODO |
Cosmetic |
OK |
90 |
Should we add this finish exception to the list of exceptions for job? Add as TODO |
Cosmetic |
OK |
108-109 |
Should we add this exception thrown during processing to the list of exceptions for job? Add as TODO |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 626
Lines |
Description |
Classification |
Status |
48 |
language: "resent" i.e. forwarded |
Cosmetic |
POSTPONE |
74 |
Rename con as jmscon |
Cosmetic |
POSTPONE |
120 |
Consider checking that inbMsg is not null |
Cosmetic |
POSTPONE |
144 |
Consider checking that beMsg is not null |
Cosmetic |
POSTPONE |
176 |
Consider checking that hbMsg is not null |
Cosmetic |
POSTPONE |
192 |
'o' is not a very good variable name og an observable object. Use 'obs' or equivilent; |
Cosmetic |
POSTPONE |
202, 206, 212 |
Add respectively "an", "an", and "a" after "Received" |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage.java', revision 544
Lines |
Description |
Classification |
Status |
45 |
"to identifier for" => "to identify" |
Cosmetic |
POSTPONE |
48 |
Rename "BA_ApplicationId" as "BitarchiveApplicationId" |
Cosmetic |
POSTPONE |
59-61 |
three empty lines in a row. Delete two of them. |
Cosmetic |
POSTPONE |
73 |
rename to BitarchiveApplicationId |
Cosmetic |
POSTPONE |
76 |
"he remote file" => "The remote file" |
Cosmetic |
POSTPONE |
177 |
Add ArgumentNotValid check |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableJarBatchJob.java', revision 760
Lines |
Description |
Classification |
Status |
116 |
Add a JAVA_PACKAGE_SEPARATOR constant for "." Add a DIRECTORY_SEPARATOR constant for "/" |
Cosmetic |
OK |
119 |
Replace "No data loaded!!!!!" with "No data loaded for class with name '" + className + "'." |
Cosmetic |
OK |
157 |
Check argument os |
Cosmetic |
OK |
170 |
'Cannout' = spelling error. |
Cosmetic |
OK |
192 |
"Finish up" => "Finish" |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/BatchStatus.java', revision 544
Lines |
Description |
Classification |
Status |
35 |
Delete line |
Cosmetic |
POSTPONE |
58-59 |
".., that run this batchJob" => ".., that ran this batch job" |
Cosmetic |
POSTPONE |
66-70 |
Check for ArgumentNotValid. |
Cosmetic |
POSTPONE |
90 |
Make "ALL_BITARCHIVE_APPS" a constant |
Cosmetic |
POSTPONE |
160 |
".. to copy into '" => ".. to copy into target file '" |
Cosmetic |
POSTPONE |
161 |
"batch job on" => "batch job run on " |
Cosmetic |
POSTPONE |
168 |
"so this" => so this method |
Cosmetic |
POSTPONE |
187 |
the variable 'stream' cannot be printed out. Just write "append to output stream". |
Cosmetic |
POSTPONE |
188 |
"batch job on" => batch job run on bitarchive application |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/common/utils/cdx/CDXUtils.java', revision 775
Lines |
Description |
Classification |
Status |
57 |
Check args arcfile and cdxstream |
Cosmetic |
POSTPONE |
59 |
Remove superfluous whitespace |
Cosmetic |
POSTPONE |
75 |
Remove superfluous whitespace |
Cosmetic |
POSTPONE |
101-105 |
replace with: ArgumentNotValid.checkTrue(arcFileDirectory.isDirectory() && arcFileDirectory.canRead(), "..."); |
Cosmetic |
POSTPONE |
106-110 |
replace with: ArgumentNotValid.checkTrue(cdxFileDirectory.isDirectory() && cdxFileDirectory.canRead(), "..."); |
Cosmetic |
POSTPONE |
140 |
could be written if (!exceptions.isEmpty()) |
Cosmetic |
POSTPONE |
Comments on file 'trunk/build.xml', revision 762
Lines |
Description |
Classification |
Status |
33-36, 71-73 |
Only three of the four new values are included. Add the fourth |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive.java', revision 774
Lines |
Description |
Classification |
Status |
106 |
ArgumentNotValid.checkNotNegative on 'index' ? |
Cosmetic |
POSTPONE |
112 |
don't use 'arcfile' before it is checked for 'ArgumentNotValid' on the following line. |
Cosmetic |
POSTPONE |
136-140 |
Remove uncommented code |
Cosmetic |
POSTPONE |
183 |
check for ArgumentNotValid before use of argument-variables. |
Cosmetic |
POSTPONE |
261-264 |
Identical messages, use a string. |
Cosmetic |
POSTPONE |
318 |
Move log after argument validation |
Cosmetic |
POSTPONE |
338 |
Should this not be synchronized to avoid multiple callers beginning the construction of a new Bitarchive object |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient.java', revision 635
Lines |
Description |
Classification |
Status |
52, 56, 58-60 |
Javadoc. |
Cosmetic |
OK |
76 |
Add ArgumentNotValid check for file, and file existence |
Cosmetic |
OK |
90 |
make ArgumentNotValid checks for arcfile (Not Null Not empty), and index (not negative) |
Cosmetic |
OK |
123 |
Note that this argument is not used in this implementation |
Cosmetic |
OK |
128 |
check for Argument not valid? |
Cosmetic |
OK |
133 |
Only need to write "batch" once |
Cosmetic |
OK |
143 |
Check for ArgumentNotValid. |
Cosmetic |
OK |
147-148 |
Remove uncommented code |
Cosmetic |
OK |
163 |
Write "batch job" instead of just "batch" |
Cosmetic |
OK |
188-190 |
Throw an NotImplementedException |
Cosmetic |
OK |
199-200 |
Throw an NotImplementedException |
Cosmetic |
OK |
213-214 |
check for ArgumentNotValid. |
Cosmetic |
OK |
215 |
note in the javadoc, that these variables are not used |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin.java', revision 774
Lines |
Description |
Classification |
Status |
52 |
Javadoc. |
Cosmetic |
OK |
164, 343 |
use instead files.toArray(new File[files.size]) |
Cosmetic |
OK |
164 |
Too long line |
Cosmetic |
OK |
262 |
Add meaning that the given File object exists, is a directory, and is writable by this process. |
Cosmetic |
OK |
298-305 |
The 'j' is not used explicit -> change to "for (File file : filesHere)" and use 'file' instead of 'filesHere[j]'. |
Cosmetic |
OK |
300 |
replace 'filesHere[j]' with "filesHere[j].getAbsolutePath()"? |
Cosmetic |
OK |
319 |
Validate regexp as Not null and Not empty String |
Cosmetic |
OK |
329-330 |
Replace "Non-file" with "Not a regular File" |
Cosmetic |
OK |
364 |
"Corrupt" => "Possibly corrupt" |
Cosmetic |
OK |
370 |
Consider logging at trace |
Cosmetic |
OK |
388-395 |
use for (File file : files) { ... } |
Cosmetic |
OK |
404-412 |
use for (File file : files) { ... } |
Cosmetic |
OK |
421-429 |
use for (File file : files) { ... } |
Cosmetic |
OK |
471-474 |
Different messages. |
Cosmetic |
POSTPONE |
479-480 |
Why use two line for this? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 554
Lines |
Description |
Classification |
Status |
General |
Should this also read the MINSPACELEFT setting used by the Bitarchives? Add as TODO |
Cosmetic |
POSTPONE |
71-72 |
Javadoc. |
Cosmetic |
POSTPONE |
79 |
Add a log.info message, that the DEFAULT_DIR_NAME is included as one of the fileDirs |
Cosmetic |
POSTPONE |
105 |
throws also IllegalState. Either add IllegalState, or remove the others. |
Cosmetic |
POSTPONE |
175 |
Mention that the location argument is unused in this implementation |
Cosmetic |
POSTPONE |
225 |
Perhaps also check for '!filesInDir.isEmpty' to aviod making a unnecessary call to the 'files.addAll' method. |
Cosmetic |
POSTPONE |
248 |
Remove uncommented code |
Cosmetic |
POSTPONE |
260-262 |
Throw NotImplementedException, and add note about this to javadoc |
Cosmetic |
POSTPONE |
271-272 |
Throw NotImplementedException and add note about this to javadoc |
Cosmetic |
POSTPONE |
279-280 |
Add that bitarchiveName is unused in this implementation of ArcRepositoryClient And is therefore not checked |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/common/utils/arc/ARCBatchJob.java', revision 544
Lines |
Description |
Classification |
Status |
45 |
remove empty line |
Cosmetic |
POSTPONE |
97 |
"if file" => if file was |
Cosmetic |
POSTPONE |
135-140 |
This is catched by the outer try-catch, and handled exactly the same way. Remove try-catch |
Minor |
OK |
139 |
Add comment: "This aborts the processing of this ARC-file" |
Cosmetic |
REJECTED |
160 |
log here: Unexpected exception caught. The processing of the rest of the arcrecords aborted |
Cosmetic |
POSTPONE |
193-195 |
Javadoc this, and argument not valid (or describe why not). |
Cosmetic |
OK |
206, 209 |
Are we supposed to write the 'throws ArgumentNotValid' for all the methods, which makes the ArgumentNotValid check? NO Because then it is needed in a lot of methods. |
Cosmetic |
OK |
208 |
Check 'ArgumentNotValid' on 'arcfile' (checkNotNull) and 'index' (checkNotNegative). |
Cosmetic |
REJECTED |
236-238 |
Javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java', revision 548
Lines |
Description |
Classification |
Status |
42 |
Javadoc. |
Cosmetic |
POSTPONE |