= [wiki:ReviewHeuristicLab3.3.0Code HeuristicLab 3.3.0 Code Review] - !HeuristicLab.Core-3.3 =

== Reviewer: mkommend ==

 * '''General''': Every generic class which implements {{{IItem}}} should provide a better {{{ItemName}}} and {{{ItemDescription}}}, with the generic type parameters specified.
   * swagner: Done in r3822.
 * '''General''': A refactoring of the deep cloning mechanism should be considered. The method {{{public virtual IDeepCloneable Clone(Cloner cloner)}}} should be split into two different methods, one for creating the object and one which sets all fields. The advantage of this splitting is that dynamically binded methods and with base calls could be used to set the properties.
   * swagner: Usually cloning is done by calling the dynamically bound method `Clone(Cloner cloner)` which makes a base call to set all data members along the inheritance chain. Only in some special cases (e.g. `Variable`) this pattern is not followed to gain performance, as instance creation using `Activator.CreateInstance` (done in `DeepCloneable`) is quite expensive. Therefore, the deep cloning mechanism has not to be refactored in my opinion.

 * '''IEngine''': {{{OperatorGraph}}} should only contain a public getter but no public setter.
   * swagner: The public setter is provided to be able to exchange the operator graph of an engine easily. This feature will be used in the future in order to be able to reuse an engine instance for executing multiple different algorithms.

 * '''!ReferenceEqualityComparer''': The {{{ReferenceEqualityComparer}}} is used as a private class in the {{{Cloner}}} and {{{ChangedEventArgs}}}. Consider extracting the {{{ReferenceEqualityComparer}}} as an internal class or into !HeuristicLab.Common.
   * swagner: Moved `ReferenceEqualityComparer` to !HeuristicLab.Common in r2790.

 * '''!ChangedEventArgs''': A ctor with {{{public ChangedEventArgs(IEnumerable <object> changedObjects)}}} should be provided.
   * swagner: Done in r2790.

 * '''Engine''': Why is the property {{{GlobalScope}}} saved in a variable of type {{{Scope}}} and not {{{IScope}}} (line 69)? 
   * swagner: Changed the type of the local variable to store the global scope to `IScope` in r2790.
 * '''Engine''': A better name for the method {{{public void Initialize()}}} (line 166) and the according event handler should be chosen. Initialized suggests that the method should only be called once after the creation of an instance. A solution would be to make the {{{Initialize}}} method private and a public {{{Reset}}} method which calls the {{{Initialize}}} method.
   * swagner: Renamed `Initialize` to `Prepare` and `Initialized` to `Prepared` in r2790.
 * '''Engine''': Currently the elapsed execution time is calculated by subtracting `DateTime.Now` - lastUpdated. IMHO using the `StopWatch` class would be more appropriate for this task.

 * '''!ExecutionContext''': The {{{private IOperator op;}}} should be renamed to operator to match the corresponding property.
   * swagner: This is not allowed as `operator` is a reserved key word.

 * '''!NamedItemCollection''': Contains a misused property {{{private object RestoreEvents}}}. The functionality is used to register events on contained items after the object has been restored by the persistence framework. In my opinion the persistence framework should provide a more general mechanism to perform configurations / initialization tasks after the object creation.
   * swagner: Changed in r3287. Used a `StorableHook` instead to register events after deserialization.

 * '''!NamedItem''': The field {{{protected string name}}} should be private instead of protected. The intention was obviously to provide access to the field without firing events. However the field is only used in clone methods and ctors of derived classes and at this time no events listeners are registered. Hence the field could be made private without side effects. The same arguments apply for {{{protected string description field}}}.
   * swagner: Done in r2790.

 * '''!NamedItemCollection''': {{{NamedItemCollection}}} should implement {{{IItem}}}, because then the {{{IItem}}} implementation could be removed from the derived classes {{{VariableCollection}}} and {{{ParameterCollection}}}.
   * swagner: Done in r2790.

== Reviewer: gkronber ==
 * Implementation of `Clone()` in `NamedItemCollection` assumes that any class extending `NamedItemCollection` has a constructor with one argument of type `IEnumerable<T>`. This cannot be enforced in `NamedItemCollection` and leads to exceptions if a constructor with this signature is not available.
   * swagner: Changed in r3286. The parameterless constructor is now used to instantiate the clones of all types of item collections.