= 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 ||