= Review (MG-5): WAF to WARC =
|| Author || Henrik Kirk ||
|| Moderator || Henrik Kirk ||
|| State || Review ||
== Objectives ==
{{{

}}}
'''Total Time Used (Coding,Documentation,Review)''':
{{{ 
Time use (Coding,Documentation,Review) 
HBK: 0.5
JOLF: 0.5
}}}

'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| Change input/output names in method calls to be consistent. || Cosmetic || OK ||


=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/io/Constants.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 1 || package after license || Cosmetic || OK ||
|| 22 || Header is wrong. || Cosmetic || OK ||
|| 29-30 || javadoc || Cosmetic || OK ||
|| 36-39 || what does these mean ? Javadoc. || Cosmetic || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/nbWAF2WARC/FindAndMoveWAFFilesApp.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 12-32 || This header is usually at first line, before package and imports. || Cosmetic || OK ||
|| 32 || Header is wrong || Cosmetic || OK ||
|| 33 || javadoc || Cosmetic || OK ||
|| 35-36 || Why are these directories stored as strings. It would be more practical to store them as files. Also for validating their existence. || Minor || OK ||
|| 35-36 || javadoc || Cosmetic || OK ||
|| 38 || javadoc || Cosmetic || OK ||
|| 40 || Shouldn't this be 'System.err' ? || Cosmetic || OK ||
|| 53 || You do not test whether they exist, only whether the argument values are null or empty. || Cosmetic || OK ||
|| 61 || A 'NullPointerException' is not a 'FileNotFoundException'. || Cosmetic || OK ||
|| 71 || Comment about what this does. Something like: 'Retrieves a list of all the files within the rootDir'. If that is what is done here. || Cosmetic || OK ||
|| 73 || Codestyle: space before ':' || Cosmetic || OK ||
|| 75-77 || Is this the same as file.getName() ? Commentary instead! || Cosmetic || OK ||
|| 80-81 || double empty line. || Cosmetic || OK ||
|| 91-96 || You throw away the exception and does not terminate. Is this intentional? Just write out exception stack trace! || Cosmetic || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/nbWAF2WARC/NBWAF2WARC.java', revision 1.3 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 13-33 || Again, the header should be before package and imports. || Cosmetic || OK ||
|| 33 || Header is wrong || Cosmetic || OK ||
|| 34 || javadoc || Cosmetic || OK ||
|| 40, 43, 47 || This looks quite funny. The Java-doc says that one kind of exception is thrown, the method definition claims to throw another and the method itself throws a third kind of exception. || Cosmetic || OK ||
|| 66 || Make method take files insteade of strings || Minor || OK ||
|| 69 || Make a comment about what you do here. || Cosmetic || OK ||
|| 82 || How can 'prefix' become 'null'? Input arguments to 'main' cannot be null. ArgumentNotValid checks! || Cosmetic || OK ||
|| 102-104 || In which scenario will 'warcdir' not be a directory? And what consequences would it have? || Minor || OK ||
|| 108-112 || Please comment about what this command does, and what the constant arguments means. || Cosmetic || OK ||
|| 114-116 || Throwing the error away? Are you sure you want to continue (no new exception or a System.exit)? || Minor || OK ||
|| 127 || Perhaps also write out which WAFFile and WAFArchiveRecord are causing problems. || Cosmetic || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/io/LastRecordException.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 1 || package after license header || Cosmetic || OK ||
|| 23 || Header is wrong || Cosmetic || OK ||
|| 27-34 || javadoc || Cosmetic || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/io/WAFFile.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 8-28 || Header before package and imports. || Cosmetic || OK ||
|| 28 || Header is wrong || Cosmetic || OK ||
|| 29 || javadoc || Cosmetic || OK ||
|| 42 || perhaps change the name of the argument variable 'file' to 'filename' or 'filepath'. It would make the following prettier. Perhaps make constructor take File as argument. || Cosmetic || OK ||
|| 45 || Write the name or path of the file, which does not exist. || Cosmetic || OK ||
|| 66-69 || Throw errors way? Not even a warning about the errors.  Return false! || Minor || OK ||
|| 75-76 || Throw errors way? Not even a warning about the errors. || Minor || OK ||
|| 123, 134 || javadoc || Cosmetic || OK ||
|| 123 || This should handle errors || Cosmetic || OK ||
|| 128-130 || This does not handle the exception at all. If it should not be able to happen, then make the method throw the exception further.  Make method private || Minor || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/io/WAFArchiveRecord.java', revision 1.3 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 1 || package after license header || Cosmetic || OK ||
|| 23 || Header is wrong || Cosmetic || OK ||
|| 36, 39-47 || javadoc || Cosmetic || OK ||
|| 37-38 || double empty line || Cosmetic || OK ||
|| 53-54 || double empty line || Cosmetic || OK ||
|| 71 || todo! || Cosmetic || OK ||
|| 80 || variablename tmp is not very informative || Cosmetic || OK ||
|| 82 || Try to avoid underscore '_' in variable names. || Cosmetic || OK ||
|| 93-94 || double empty line || Cosmetic || OK ||
|| 99 || "while true" is a potential infinite loop.  It is better to iterate over the same condition as you have for your break.  Move break condition || Minor || OK ||
|| 102-108 || Comment about what this does. It seems to only test the first piece of data which is extracted from the inputstream. And an error message || Cosmetic || OK ||
|| 104 || System.err instead? || Cosmetic || OK ||
|| 136 || odd empty line || Cosmetic || OK ||
|| 173 || Why a comment about System.out.println ? || Cosmetic || OK ||
|| 178-181 || More odd commented System.out.println. Also, the "if statement" does nothing even if the argument is valid. || Cosmetic || OK ||
|| 188 || another commented System.out.println || Cosmetic || OK ||
|| 198 || commented System.out.println || Cosmetic || OK ||
|| 209, 218, 222, 226, 234 || javadoc || Cosmetic || OK ||
|| 238-246 || remove? || Cosmetic || OK ||
|| 248, 267, 271 || javadoc || Cosmetic || OK ||
|| 257-259 || this does the same as the following IOException. Why not combine the to catches of IOException? e.g.  try {    try {   ....    } catch FileNotFound { ... CanonicalPath ... } } catch IOException { ... } || Minor || OK ||
|| 260-261 || odd newline. || Cosmetic || OK ||
|| 279-311 || "Boring getter methods" also need javadoc. || Cosmetic || OK ||
|| 312-313 || odd double empty line || Cosmetic || OK ||
|| 325 || What is the purpose of this "Helper method" ? It cannot help with every problem, so which kind of problems can it help with. || Cosmetic || OK ||
|| 342-346 || Does this make sense to you? You retrieve the index for the position of Constant.POST. If the index is different from -1 (meaning that it was found), then the index is returned. Otherwise you return -1. Why not replace with the following which seems to do the same:  return ByteUtils.findByteSequence(tmp_array, Constants.Post, 2)); || Minor || OK ||
|| 347 || odd empty line || Cosmetic || OK ||
|| 356 || odd empty line || Cosmetic || OK ||
|| 369-370 || double empty line || Cosmetic || OK ||
|| 373 || System.err instead? || Cosmetic || OK ||
=== Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/WAF2WARC/nbWAF2WARC/Constants.java', revision 1.1 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 1 || package after license header || Cosmetic || OK ||
|| 22 || Header is wrong || Cosmetic || OK ||
|| 29-39 || javadoc? || Cosmetic || OK ||