= Review (NS-19): Bug 934 -  removal of Sidekick from NetarchiveSuite =
|| Author || Søren ||
|| Moderator || Søren ||
|| State || Closed ||
== Objectives ==
{{{
Review of files:
/trunk/src/dk/netarkivet/harvester/settings.xml (removed isRunning setting after line 145)
/trunk/src/dk/netarkivet/harvester/HarvesterSettings.java 552 (removed constant for isRunning setting at the end)
/trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerServer.java, r551, 134-135, 151-212, 284-301, 446-459, 659-661
/trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerClient.java, r551,63-64 (removal of wrong javadoc throws clause), 76 (changed modifier order)
/trunk/src/dk/netarkivet/harvester/harvesting/HarvestControllerApplication.java (ALL LINES, all lot of code removed)
/trunk/scripts/simple_harvest/harvest.sh, r551, 137-171
/trunk/scripts/simple_harvest/settings.xml, r552, (removed isRunning, and topLevelDomains (already in defaults)
Note that the following files were deleted i r551:
===================================
/trunk/src/dk/netarkivet/harvester/sidekick/DefaultMonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/HarvestControllerServerMonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/MonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/SideKick.java
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
ELZI:  0.5
SVC:  1.5
JOLF: 0.5
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||


=== Comments on file 'trunk/scripts/simple_harvest/harvest.sh', revision 551 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/scripts/simple_harvest/settings.xml', revision 552 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerServer.java', revision 642 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 121-122 || Naming the JMSConnection variable 'con' is confusing. It could just as well refer to other variables, e.g. the HarvestController. || Cosmetic || OK ||
|| 665 || Why not use the function resumeAcceptingJobs() instead of just setting the running to false? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HarvestControllerApplication.java', revision 551 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerClient.java', revision 551 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 70-78 || I thought that a construction factory ensured that only one instance exists. This function allows several identical HarvestControllerClient instances to exist at the same time. Is it the purpose of this method? And if so then why not allow regular (public) use of the constructor? || Cosmetic || REJECTED ||
|| 54 || It is confusing to see a variable name 'con', since it can refer to several things, e.g. a connection, a controller, etc. || Cosmetic || OK ||
.