Review (NS-159): Wayback Aggregator

Author	Mikis Seth Sørensen
Moderator	Mikis Seth Sørensen
State	Closed

Objectives

Review af Aggregator kode og javadoc/pakke beskrivelser.

Summary

Discussion about where to validate arguments in methods who delegate to other methods and how to access private methods from unittest. Mikis will escalate the discussing to a wider forum

Total Time Used (Coding,Documentation,Review):

Time use (Coding,Documentation,Review)
JOLF: 0.3
MSS: 5

General comments:

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/wayback/aggregator/AggregatorApplication.java', revision 1428

Lines	Description	Classification	Status
1	bad header	Cosmetic	NOTOK

Comments on file 'trunk/src/dk/netarkivet/wayback/aggregator/AggregationWorker.java', revision 1470

Lines	Description	Classification	Status
General	I like the way logging is used here. Saves a lot of processing trying to handle log-entries, which has been disabled.	NA	NOTOK
1-4	odd header	Cosmetic	NOTOK
51-67	javadoc?	Cosmetic	NOTOK
144	more comments about what is done would make it easier to understand.	Cosmetic	NOTOK
149-158	why not have the 'if' before the 'for loop'? even before the declaration of the filesToProcess!	Cosmetic	NOTOK
150	Remember to check is valid file	NA	NOTOK
196	missing javadoc	Cosmetic	NOTOK
203	missing javadoc	Cosmetic	NOTOK
204-236	comment about what is done here.	Cosmetic	NOTOK
237	odd empty line	Cosmetic	NOTOK
240	missing javadoc	Cosmetic	NOTOK
266-267	odd empty lines	Cosmetic	NOTOK
275	missing javadoc	Cosmetic	NOTOK

Comments on file 'trunk/src/dk/netarkivet/wayback/aggregator/IndexAggregator.java', revision 1470

Lines	Description	Classification	Status
41	missing javadoc.	Cosmetic	NOTOK
52	argument validation?	Minor	NOTOK
57	unfinished sentence. Missing dot at the end of line.	Cosmetic	NOTOK
61	odd empty line	Cosmetic	NOTOK
62	Argument validation?	Minor	NOTOK
90-108	use comments to describe what is done here.	Cosmetic	NOTOK
100	is this intended? Write that you want to retrieve the value from settings instead of the actual value from the settings?	Major	NOTOK
101	also check isEmpty, otherwise the following code is irrelevant.	Cosmetic	NOTOK

Comments on file 'trunk/src/dk/netarkivet/wayback/WaybackSettings.java', revision 1428

Lines	Description	Classification	Status
156, 160, 164, 168, 176, 183	This seems to break our code-style. I have not previously seen the use of '_' or '-' in settings. We usually use uppercase letters to separate words in the settings paths. E.g. aggregation_interval => aggregationInterval	Cosmetic	NOTOK