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 |