= Review (MG-9): Review of code to migrate from pligtsys archivingsystem to warc =
|| Author || Søren ||
|| Moderator || Søren ||
|| State || Closed ||
== Objectives ==
{{{
QA of the code-base used to migrate from old legal deposit system (pligtafleveringssystem 1998-2005) to WARC.
Uses WARCWriter framework created by Henrik Kirk (SB).
The essential files are PligtToArcOrWarc, Pligt2WARC, PligtArchiveRecord, Fil (encapsulates metadata from one stored URI); Vaerk (encapsulates metadata about a publication); Repr (encapsulates metadata about one representation)
}}}
== Summary ==
{{{
Followup by SVC
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
SVC: 8 MD
JOLF: 1 MD
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| There seems to be more or less no argument validation. || Cosmetic ||REJECTED ||


=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/DatabaseTypes.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 8-18 || javadoc || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/WarcConstants.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 12-22, 24-45 || odd indentation || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/Repr.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || No javadoc. || Cosmetic || OK ||
|| 28 || space between ',' and 'String' || Cosmetic || OK ||
|| 35-36 || Why do you have these indifferent variables. || Cosmetic || OK ||
|| 57-58 || new line between || Cosmetic || OK ||
|| 60 || Remove || Cosmetic || OK ||
|| 66-67 || odd double line || Cosmetic || OK ||
|| 71 || line beyond 80 characters (if I am not mistaken) || Cosmetic || OK ||
|| 88-89 || Remove || Cosmetic || OK ||
|| 92-93 || indentation || Cosmetic || OK ||
|| 96 || is this some kind of exception? A comment about what happens here would be nice. || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/Vaerk.java', revision 1.4 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 82-83 || new line in between || Cosmetic || OK ||
|| 87-88 || new line in between || Cosmetic || OK ||
|| 88-96 || Why the values 10, 20 and 30? why not 1, 2, 3? || Cosmetic || OK ||
|| 96-97 || new line in between || Cosmetic || OK ||
|| 105-117 || javadoc || Cosmetic || OK ||
|| 119-125 || Are the comment connected to the variable? If no, then javadoc the variable and move the comment to a more logical location. If yes, then make it into javadoc instead of just a comment. || Cosmetic || OK ||
|| 130 || Resulset => Resultset || Cosmetic || OK ||
|| 145-146 || remove || Cosmetic || OK ||
|| 149-150 || remove || Cosmetic || OK ||
|| 164-166 || Do you really want to print this out? || Cosmetic || OK ||
|| 193 || Do you really want to print this out? || Cosmetic || OK ||
|| 203 || odd empty line || Cosmetic || OK ||
|| 204-207 || SQLException extends Exception, and it does not make any sense to handle them in the same way. The first catch is superfluous. || Cosmetic || OK ||
|| 211-215 || Odd javadoc || Cosmetic || OK ||
|| 220-224 || Odd javadoc || Cosmetic || OK ||
|| 236-237 || Odd javadoc || Cosmetic || OK ||
|| 352 || can this be false? You make a count, would this not return 0 if the table is empty? || Cosmetic || OK ||
|| 354 || remove || Cosmetic || OK ||
|| 356 || Should this not be System.err ? || Cosmetic || OK ||
|| 359 || Is this some kind of exception? || Cosmetic ||REJECTED ||
|| 368-372 || dummy javadoc || Cosmetic || OK ||
|| 383 || why 1? Also remove the last comment, it does not seem to be true any more. || Cosmetic || OK ||
|| 385 || Is this an exception? || Cosmetic || OK ||
|| 393-396 || odd javadoc || Cosmetic || OK ||
|| 401-404 || odd javadoc || Cosmetic || OK ||
|| 409-413 || odd javadoc. Should document that it might try to exit || Cosmetic || OK ||
|| 418-441 || Write what you do here. You split, and split, and split again! || Cosmetic || OK ||
|| 423-424 || This should be System.err || Cosmetic || OK ||
|| 432-433 || This should be System.err || Cosmetic || OK ||
|| 438-439 || This should be System.err || Cosmetic || OK ||
|| 442-443 || You do not catch the potential NumberFormatException from Long.parseLong || Cosmetic || OK ||
|| 449-451 || not complete javadoc. return? || Cosmetic || OK ||
|| 460-461 || indentation || Cosmetic || OK ||
|| 465 || javadoc || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/PligtToArcOrWarc.java', revision 1.5 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 26-31 || should this be the javadoc? It should not contain specific machine name, path, and IP. || Cosmetic || OK ||
|| 36-37 || javadoc || Cosmetic || OK ||
|| 45 || Are we still testing? || Cosmetic || OK ||
|| 54-56 || The comments and the values look like testing variables. || Cosmetic || OK ||
|| 56 || javadoc || Cosmetic || OK ||
|| 65-67 || Since this is for test only, then there should be an If statement around it. if(TEST_MODE) {     for(int i..... } || Cosmetic ||REJECTED ||
|| 72-75 || javadoc || Cosmetic || OK ||
|| 85-86 || Is this not a valid reason to exit? || Cosmetic || OK ||
|| 95-96 || line longer than 80 characters. Also space between conToDbPeriodMellemreg and '='. || Cosmetic || OK ||
|| 123 || remove || Cosmetic || OK ||
|| 135-136 || odd empty double line || Cosmetic || OK ||
|| 158 || Are this done repeatedly, or do you only want to handle 200 items in total? || NA || OK ||
|| 165 || I do not get this line. What is its purpose? || Cosmetic || OK ||
|| 168 || is this an exception? || Cosmetic || OK ||
|| 169-170 || remove || Cosmetic || OK ||
|| 175-177 || javadoc || Cosmetic || OK ||
|| 188-191 || Is this TODO done? || NA ||POSTPONED ||
|| 201 || What does this line refer to? || NA || OK ||
|| 209 || remove? If so, then also the comment || Cosmetic || OK ||
|| 212 || You just rethrow the exception. It seems superfluous when you could just let the exception fall through. || Cosmetic || OK ||
|| 221-227 || Move System.out.println(v) to be prior to the if statement, then remove the else. || Cosmetic || OK ||
|| 228-229 || double empty line || Cosmetic || OK ||
|| 263 || remove || Cosmetic || OK ||
|| 264 || looks like test data || Cosmetic || OK ||
|| 266 || remove || Cosmetic || OK ||
|| 270 || What does the 2000000L represent? the size of the output files, the amount of output files, or something else? || NA || OK ||
|| 276-279 || Both writing to log and system out? Since the message is the same it should be defined as a string before use. || Cosmetic || OK ||
|| 313-315 || why not return potentialFile as soon as it is found? || NA || OK ||
|| 316-318 || what a pretty else statement. Remove || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/PligtArchiveRecord.java', revision 1.4 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 29 || javadoc || Cosmetic || OK ||
|| 34-222, 237-256 || Odd indentation || Cosmetic || OK ||
|| 42-43 || unused parameters in javadoc || Cosmetic || OK ||
|| 75-77 || 'return' missing from javadoc || Cosmetic || OK ||
|| 82-84 || odd javadoc || Cosmetic || OK ||
|| 95 || missing javadoc || Cosmetic || OK ||
|| 99-100 || new line break in between || Cosmetic || OK ||
|| 124-126 || odd javadoc || Cosmetic || OK ||
|| 131-133 || odd javadoc || Cosmetic || OK ||
|| 141-147 || odd javadoc || Cosmetic || OK ||
|| 154 || This looks like the limit of a two byte integer. Why has this value been chosen? || Cosmetic || OK ||
|| 160 || Perform the TODO || Cosmetic || OK ||
|| 171-172 || Does this throw the stacktrace away? || NA || OK ||
|| 188 || missing javadoc || Cosmetic || OK ||
|| 192 || missing javadoc || Cosmetic || OK ||
|| 203 || missing javadoc || Cosmetic || OK ||
|| 204-205 || Perform TODO and return something valid instead of null? || NA || OK ||
|| 208 || missing javadoc || Cosmetic || OK ||
|| 212 || missing javadoc || Cosmetic || OK ||
|| 216 || missing javadoc || Cosmetic || OK ||
|| 222 || missing javadoc || Cosmetic || OK ||
|| 232 || remove || Cosmetic || REJECTED ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/Fillink.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Consider deprecating this class. || Cosmetic || OK ||
|| 23-31 || javadoc || Cosmetic || OK ||
|| 33 || missing javadoc || Cosmetic || OK ||
|| 33 || Shouldn't this be public? || Cosmetic ||REJECTED ||
|| 57-62 || Why handle these exceptions different?  Move System.out prior to e.printStackTrace. Change System.out to System.err || Cosmetic || OK ||
|| 65-67 || odd javadoc || Cosmetic || REJECTED ||
|| 72-74 || odd javadoc || Cosmetic || REJECTED ||
|| 79-81 || odd javadoc || Cosmetic || REJECTED ||
|| 86-88 || odd javadoc || Cosmetic || REJECTED ||
|| 93-95 || odd javadoc || Cosmetic || REJECTED ||
|| 100-102 || odd javadoc || Cosmetic || REJECTED ||
|| 107 || missing javadoc || Cosmetic || OK ||
|| 112-114 || odd indentation || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/Pligt2WARC.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 13-17 || javadoc || Cosmetic || OK ||
|| 19 || odd indentation || Cosmetic || OK ||
|| 31-33 || There is no need to create a string with the error message, when it is only used once.  I think it is the first time I've seen anybody actively throw a NullPointerException. They are usually only thrown unintentionally. || Cosmetic || OK ||
|| 36-38 || There is no need to create a string with the error message, when it is only used once. || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/PeriodicalIssue.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 35-37 || odd javadoc || Cosmetic ||REJECTED ||
|| 42-44 || odd javadoc || Cosmetic || REJECTED ||
|| 49-51 || odd javadoc || Cosmetic ||REJECTED ||
|| 56-58 || odd javadoc || Cosmetic ||REJECTED ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/WarcFile.java', revision 1.2 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add @deprecated: Use HBK framework instead || Minor || OK ||
|| 24-28 || javadoc || Cosmetic || OK ||
|| 31-33 || not finished javadoc || Cosmetic || OK ||
|| 43 || is this a good max size?  what is the measure? entries, bits, bytes, kB, MB, GB, TB? || Cosmetic || OK ||
|| 61 || javadoc || Cosmetic || OK ||
|| 63 || do you really want to leave the FIXME hanging? Why not fix it. || Cosmetic || OK ||
|| 71-72 || remove? || Cosmetic || OK ||
|| 75 || javadoc || Cosmetic || OK ||
|| 76 || Are you sure you want to print? || Cosmetic || OK ||
|| 77-78 || does this make sense?  First close, then get the file. || Cosmetic || OK ||
|| 82 || javadoc || Cosmetic || OK ||
|| 86 || javadoc || Cosmetic || OK ||
|| 89-90 || double empty line. Remove one (a random one!) || Cosmetic || OK ||
|| 91-92 || The comment might be a good description, but it is no javadoc. || Cosmetic || OK ||
|| 97 || The stacktrace is thrown away. Is this intended? || Cosmetic || OK ||
|| 101-102 || double empty line || Cosmetic || OK ||
|| 103-105 || unfinished javadoc || Cosmetic || OK ||
|| 109-126 || 18 empty lines in a row. TODO delete them. || Cosmetic || OK ||
=== Comments on file 'svc/migrationToArcOrWarc/src/dk/kb/pligt/Fil.java', revision 1.5 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 23 || javadoc || Cosmetic || OK ||
|| 24-35 || missing javadoc || Cosmetic || OK ||
|| 36-40 || remove? || Cosmetic || OK ||
|| 42-53 || Javadoc, and comment on what is actually done here. || Cosmetic || OK ||
|| 55-59 || remove? perhaps only some of the lines. || Cosmetic || OK ||
|| 61-76 || javadoc and comment on what is actually done here. || Cosmetic || OK ||
|| 81-83 || javadoc: missing parameter description. || Cosmetic || OK ||
|| 87, 89 || remove? || Cosmetic || OK ||
|| 104 || remove? || Cosmetic || OK ||
|| 112 || remove? || Cosmetic || OK ||
|| 133-134 || remove? || Cosmetic || OK ||
|| 143-144 || remove? || Cosmetic || OK ||
|| 154-155 || remove? || Cosmetic || OK ||
|| 181-184 || Catching of the SQLException is superfluous, when it is handled the same way as Exception || Cosmetic || OK ||
|| 185 || odd indentation || Cosmetic || OK ||
|| 188 || missing javadoc || Cosmetic || OK ||
|| 192 || missing javadoc || Cosmetic || OK ||
|| 196 || missing javadoc || Cosmetic || OK ||
|| 200 || missing javadoc || Cosmetic || OK ||
|| 204 || missing javadoc || Cosmetic || OK ||
|| 208 || missing javadoc || Cosmetic || OK ||
|| 213 || missing javadoc || Cosmetic || OK ||
|| 217 || missing javadoc || Cosmetic || OK ||
|| 221 || missing javadoc || Cosmetic || OK ||
|| 225 || missing javadoc || Cosmetic ||OK ||
|| 229 || missing javadoc || Cosmetic || OK ||
|| 233 || missing javadoc || Cosmetic || OK ||
|| 278 || perform TODO || Cosmetic || OK ||
|| 356-357 || double empty line || Cosmetic || OK ||
|| 360-361 || double empty line || Cosmetic || OK ||