Review (MG-3): Changed java package names, what a mess in Intellij.
Author |
Henrik Kirk |
Moderator |
Henrik Kirk |
State |
Review |
Objectives
MAIN:hbk:20090929112545: Changed java package names, what a mess in Intellij.
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) HBK: 0.5 MD SVC: 1MD
General comments:
Description |
Classification |
Status |
|| Use the following fileheader when adding fileheaders to your CVS files
|| Cosmetic || OK |||| You need to write @param software Software used for this harvest/crawl instead of just @param software used for this harvest/crawl.
Try generating the javadoc, and see my point. || Cosmetic || OK ||
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/Archive2WARC.java', revision 1.3
Lines |
Description |
Classification |
Status |
57 |
Indicate what the suffix of the files generated from the Warc-writer is? Should this suffix be a parameter to this method. |
Cosmetic |
OK |
69 |
What constructure are we talking about. This abstract class does not have any constructures. |
Cosmetic |
OK |
70-72 |
The IllegalArgumentException is currently thrown if the parameter "af" is null. Does warcW.writeResponseRecord also throw IllegalArgumentException? |
Cosmetic |
OK |
78 |
Change to "no instance of ConverterWARCWriter created. Call OpenWARCStream first." |
Cosmetic |
OK |
83 |
spelling: diffenrent => different |
Cosmetic |
OK |
96-105 |
You need to write @param software Software used for this harvest/crawl and similarly for the other @param lines. |
Cosmetic |
OK |
117 |
See previous comment |
Cosmetic |
OK |
124 |
Spelling: haeders => headers |
Cosmetic |
OK |
149 |
Change to @throws IOException If exception is caught during the writing of the WarcInfoRecord, or if the WARC Info record have already been written and forceInfoWrite is set to false. |
Cosmetic |
OK |
162 |
Change to: Closes the WarcConverterWriter |
Cosmetic |
OK |
188 |
spelling: wehter = > whether |
Cosmetic |
OK |
206 |
Should we have checks here for validity of the metadata? add as TODO. refer to the specification |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/common/WARCDate.java', revision 1.6
Lines |
Description |
Classification |
Status |
27 |
Don't you mean GMT (Greanwich Mean Time)? |
Cosmetic |
OK |
47 |
Add a period at end of line |
Cosmetic |
OK |
48 |
spelling: contaning => containing |
Cosmetic |
OK |
50 |
spelling: formate => format |
Cosmetic |
OK |
68 |
Add that the the string looks like this: 2008-12-19T23:22:43Z format = YYYY-MM-DDTHH:mm:ssZ |
Cosmetic |
OK |
85 |
add missing period at end of line |
Cosmetic |
OK |
87 |
Add or null if lastmodified <= 0 (indicating that the file does not exist?) |
Cosmetic |
OK |
101, 103 |
Add missing period at end of line |
Cosmetic |
OK |
105, 109 |
Write in javadoc and in the exception that the number (X) is must be in the interval [0,60] |
Cosmetic |
OK |
126 |
Don't you want to check that the input has the expected format? add TODO to make a regexp for this format and check the string up against the regexp |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/io/ArchiveRecord.java', revision 1.2
Lines |
Description |
Classification |
Status |
37, 44, 51, 57, 66, 72, 80, 86, 92, 98, 104, 110, 116, 123 |
The public modifier is redundant. In a public interface, all methods are public by default |
Cosmetic |
OK |
95 |
spelling: Retrurn => return |
Cosmetic |
OK |
119-120 |
What does this javadoc sentence mean? |
Cosmetic |
OK |
121 |
Change to @return the WARCrecordType for this ArchiveRecord |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/io/AbstractArchiveRecord.java', revision 1.2
Lines |
Description |
Classification |
Status |
30, 32 |
Should these fields be optional? Or delete these lines |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/io/Constants.java', revision 1.1
Lines |
Description |
Classification |
Status |
30-39 |
Add javadoc for these constants |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/archiveConverters/ConverterWARCWriter.java', revision 1.2
Lines |
Description |
Classification |
Status |
47 |
What does the string mean? |
Cosmetic |
OK |
69 |
language: if the underlaying WARC record fails => If the writing of the WARC response record fails. |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/common/CoolStringMethods.java', revision 1.3
Lines |
Description |
Classification |
Status |
37 |
Missing period |
NA |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/common/io/CopyFile.java', revision 1.4
Lines |
Description |
Classification |
Status |
35 |
Copy file. |
Cosmetic |
OK |
43 |
Add a finally clause that close the streams |
Cosmetic |
OK |
Comments on file 'hbk/ArchiveToWARC/src/dk/sb/migration/common/io/FileUtils.java', revision 1.3
Lines |
Description |
Classification |
Status |
38 |
Add: If the given directory is a file, a list with this file included will be returned |
Cosmetic |
OK |