HeuristicLab 3.3.0 Code Review - HeuristicLab.Parameters

Reviewer: mkommend

• General: In my opinion the non-generic interfaces as well as classes, which implement / are derived from IParameter are senseless and should be removed.
  • mkommend: As discussed with swagner the non-generic interfaces are necessary to allow collections of parameters, but the non-generic classes should be removed.
• General: Is it really necessary to have an IValueLookupParameter? This type only complicates the whole situation. If it is necessary to change a value parameter to a lookup parameter this should be done in the surrounding type.
• General: Since a non-optional parameter could have the value null when it is constructed the optional parameter classes should be removed and all parameters should have the possibility of a null value.
• General: I would remove all default ctors of the parameter classes, so that a name must be specified during the construction.
• General: A renaming of OptionalXXXParameter and XXXParameter should be considered. In my opinion it is more logical if XXXParameter could be null and ObligateXXXParameter must have a value specified.

• IParameter: The property IItem ActualValue should be renamed to Value unless there is a logical reason for the naming.
• IParameter: The property IExecutionContext ExecutionContext should be moved to ILookupParameter.

• Parameter: The field protected IItem cachedActualValue should be made private and renamed to value.
• Parameter: The property public IItem ActualValue should be made virtual and renamed to Value according to the IParameter interface. These changes make the GetActualValue and SetActualValue methods unnecessary.
• Parameter: The field and property for the IExecutionContext should be removed as well as the OnExecutionContextChanged method.
• Parameter: The default ctor should be made private.
• Parameter: As stated before this class should be generic. Afterward the 'Type dataType' parameter can be removed from the ctors, because the dataType can be inferred by the generic type parameter.

• OptionalValueParameter: In the setter of the property IItem IValueParameter.Value it would be more readable to check the type with the is operator.
• OptionalValueParamter: A property IsValueAssigned or ValueAssigned, which checks if the value != null would be helpful and make the source code more readable.

• OptionalConstrainedValueParameter: This class should be derived from ValueParameter.
• ConstrainedValueParameter: The behavior of the collection event methods is a little bit confusing, because of the FirstOrDefault usage the value could become null. This is hopefully irrelevant when every parameter is allowed to have null as value.

• LookupParameter: The ActualName should be nullable to be consistent with the other parameters. If the ActualName is null no lookup will be performed and the Value will be null.
• LookupParameter: The IExecutionContext ExecutionContext property must be added and the T Value property of Parameter overloaded, as suggested above.
• LookupParameter: Is the ActualName property really necessary? Would not it be more intuitive that a LookupParameter looks after its own name?
• LookupParameter: The GetParameter method must be refactored! If the ValueLookupParameter class has been buried this method will be much easier to implement.
• LookupParameter: A null check for the ExecutionContext should be inserted in every method which accesses it.
• LookupParameter: As far as I understand the code, the TranslateName and the GetParameter method can be unified if ValueLookupParameter do not exists.
• LookupParameter: Why is the public static string TranslateName located in the LookupParameter? Is this the correct place or should it be located elsewhere (IExecutionContext)?

• SubScopesLookupParameter: In the GetActualValueMethod the name is translated by the TranslateName method. The name is translated upwards in the ExecutionContext hierarchy, but I am not sure if this behavior is intended.
• SubScopesLookupParameter: The SubScopesLookupParameter should have a property which returns a concrete ItemArray<T> as the value.
• SubScopesLookupParameter: I think it would be better to skip a value with a different type instead of throwing an exception in the GetActualValue method. In the SetActualValue a type check should also be applied to avoid exceptions.

---