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