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.
• 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.

• 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.

• ItemArray, ItemCollection, ItemList, ItemSet, NamedItemCollection: Contain 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.

• 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.