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

IssuesFromMg9 (last edited 2010-08-16 10:24:33 by localhost)