= Review (NS-37): Settings additional with focus on Channels =
|| Author || Eld Zierau ||
|| Moderator || Eld Zierau ||
|| State || Closed ||
== Objectives ==
{{{
Include channel part of review NS-33.
This main review is mainly concerns changes and clean up of setting, and implementing: Assignment B.2.2a0 - Separate the meaning of replica and physical location. All files not mentioned in the below lists only concerns:
- Move of comments from settings into javadoc
- Refactoring (replica instead of location, Id instead of name)
Channels has new basis in form of applicationId instead of http port and new meaning of replica, the main changes are made in the following files:
/trunk/src/dk/netarkivet/common/Constants.java Move language settings from constants to settings.
/trunk/src/dk/netarkivet/common/CommonSettings.java all (include JMX, web interface language)
/trunk/src/dk/netarkivet/common/webinterface/HTMLUtils.java line 205-220 language
/trunk/src/dk/netarkivet/common/distribute/Channels.java all (see diff 447 - 727)
/trunk/src/dk/netarkivet/common/distribute/ChannelID.java  all  (see diff 447 - 727)
/trunk/src/dk/netarkivet/common/distribute/arcrepository/Replica.java  all (see diff 526 - 626)
/trunk/src/dk/netarkivet/common/distribute/arcrepository/ReplicaType.java all new
/trunk/src/dk/netarkivet/common/settings.xml all - new replica settings
/trunk/conf/settings_example.xml all
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
ELZI: 10 MD
SVC: 5 MD
Earlier reviews concerning the Settings rewrite is included in these numbers
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||


=== Comments on file 'trunk/src/dk/netarkivet/common/webinterface/HTMLUtils.java', revision 637 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Badly in need of argument checking of public methods. Some methods will throw NullPointerExceptions of null arguments || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/Channels.java', revision 727 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || The channel-prefixes ALL_BA, ANY_BA, THE_BAMON, THE_SCHED, ANY_LOWPRIORITY_HACO ANY_HIGHPRIORITY_HACO, THIS_REPOS_CLIENT, THE_REPOS, ERROR, INDEX_SERVER, THIS_INDEX_CLIENT, MONITOR should all be made as final static String constants || Cosmetic || OK ||
|| 74 || Missing javadoc for the Channels constructor || Cosmetic || OK ||
|| 98-99 || Remove this comment, or add TODO before "Sort in .." || Cosmetic || OK ||
=== Comments on file 'trunk/conf/settings_example.xml', revision 626 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Run this file through deploy validation || Cosmetic || OK ||
|| 527 || Comment "applicationsInstanceId" here || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/Replica.java', revision 591 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 57 || Consider renaming "known" as "knownReplicas" || Cosmetic || OK ||
|| 105 || Null value for parameter id seems to be allowed; Mention in the javadoc, that null id is allowed, or add a CheckNotNull for id parameter  Add to javadoc: throws UnknownId if null id or unknown replica  Write to webarkivering-teknik: In need of policy for whether or not add throws of unchecked exception to the method signatures || Cosmetic || OK ||
|| 214 || The type that => The type of || Cosmetic || OK ||
|| 222 || Language: "The id that this replica" => "the id of this replica" || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/ChannelID.java', revision 727 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 124 || Have a constant for the "_" string. || Cosmetic || OK ||
|| 131 || id.equals("") => id.isEmpty() || Cosmetic || OK ||
|| 244 || Code style violation: i+1 => i + 1 || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/Constants.java', revision 637 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/ReplicaType.java', revision 592 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 10, 14, 18 || Add missing dot || Cosmetic || OK ||
|| 31 || The integer rt will never be null. Maybe replace with a checkNotNegative instead; or remove || Cosmetic || OK ||
|| 51, 54 || Replace the strings with String constants: perhaps BITARCHIVE_REPLICATYPE = "bitarchive" and CHECKSUM_REPLICATYPE = "checksum" || Cosmetic || OK ||