14033
Comment:
|
← Revision 12 as of 2010-08-16 10:24:33 ⇥
13613
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 8: | Line 8: |
Uses WARCWriter framework created by Henrik Kirk (SB). |
Uses WARCWriter framework created by Henrik Kirk (SB). |
Line 14: | Line 12: |
{{{ | {{{ |
Line 18: | Line 16: |
{{{ | {{{ |
Line 22: | Line 21: |
Line 25: | Line 23: |
|| There seems to be more or less no argument validation. || Cosmetic || NOTOK || | || There seems to be more or less no argument validation. || Cosmetic ||REJECTED || |
Line 30: | Line 28: |
|| 8-18 || javadoc || Cosmetic || NOTOK || | || 8-18 || javadoc || Cosmetic || OK || |
Line 33: | Line 31: |
|| 12-22, 24-45 || odd indentation || Cosmetic || NOTOK || | || 12-22, 24-45 || odd indentation || Cosmetic || OK || |
Line 36: | Line 34: |
|| General || No javadoc. || Cosmetic || NOTOK || || 28 || space between ',' and 'String' || Cosmetic || NOTOK || || 35-36 || Why do you have these indifferent variables. || Cosmetic || NOTOK || || 57-58 || new line between || Cosmetic || NOTOK || || 60 || Remove || Cosmetic || NOTOK || || 66-67 || odd double line || Cosmetic || NOTOK || || 71 || line beyond 80 characters (if I am not mistaken) || Cosmetic || NOTOK || || 88-89 || Remove || Cosmetic || NOTOK || || 92-93 || indentation || Cosmetic || NOTOK || || 96 || is this some kind of exception? A comment about what happens here would be nice. || Cosmetic || NOTOK || |
|| 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 || |
Line 48: | Line 46: |
|| 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 || |
|| 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 || |
Line 84: | Line 82: |
|| 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 || |
|| 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 || |
Line 115: | Line 113: |
|| 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 || |
|| 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 || |
Line 139: | Line 137: |
|| 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 || |
|| 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 || |
Line 154: | Line 152: |
|| 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 || |
|| 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 || |
Line 160: | Line 158: |
|| 35-37 || odd javadoc || Cosmetic || NOTOK || || 42-44 || odd javadoc || Cosmetic || NOTOK || || 49-51 || odd javadoc || Cosmetic || NOTOK || || 56-58 || odd javadoc || Cosmetic || NOTOK || |
|| 35-37 || odd javadoc || Cosmetic ||REJECTED || || 42-44 || odd javadoc || Cosmetic || REJECTED || || 49-51 || odd javadoc || Cosmetic ||REJECTED || || 56-58 || odd javadoc || Cosmetic ||REJECTED || |
Line 166: | Line 164: |
|| 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 || |
|| 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 || |
Line 186: | Line 184: |
|| 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 || |
|| 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 || |
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 |