= [wiki:ReviewHeuristicLab3.3.0Code 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.
----