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