| 5 | | * to be continued |
| | 5 | * mkommend: As discussed with swagner the non-generic interfaces are necessary to allow collections of parameters, but the non-generic classes should be removed. |
| | 6 | * '''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. |
| | 7 | * '''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. |
| | 8 | * '''General''': I would remove all default ctors of the parameter classes, so that a name must be specified during the construction. |
| | 9 | |
| | 10 | * '''IParameter''': The property `IItem ActualValue` should be renamed to `Value` unless there is a logical reason for the naming. |
| | 11 | * '''IParameter''': The property `IExecutionContext ExecutionContext` should be moved to `ILookupParameter`. |
| | 12 | |
| | 13 | * '''Parameter''': The field `protected IItem cachedActualValue` should be made private and renamed to `value`. |
| | 14 | * '''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. |
| | 15 | * '''Parameter''': The field and property for the `IExecutionContext` should be removed as well as the `OnExecutionContextChanged` method. |
| | 16 | * '''Parameter''': The default ctor should be made private. |
| | 17 | * '''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. |
| | 18 | |
| | 19 | * '''!OptionalValueParameter''': In the setter of the property `IItem IValueParameter.Value` it would be more readable to check the type with the `is` operator. |
| | 20 | |
| | 21 | * '''!OptionalConstrainedValueParameter''': This class should be derived from `ValueParameter`. |
| | 22 | * '''!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. |
| | 23 | |
| | 24 | * '''!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. |
| | 25 | * '''!LookupParameter''': The `IExecutionContext ExecutionContext` property must be added and the `T Value` property of `Parameter` overloaded, as suggested above. |
| | 26 | * '''!LookupParameter''': Is the `ActualName` property really necessary? Would not it be more intuitive that a `LookupParameter` looks after its own name? |
| | 27 | * '''!LookupParameter''': The `GetParameter` method must be refactored! If the `ValueLookupParameter` class has been buried this method will be much easier to implement. |
| | 28 | * '''!LookupParameter''': A null check for the `ExecutionContext` should be inserted in every method which accesses it. |
| | 29 | * '''!LookupParameter''': As far as I understand the code, the `TranslateName` and the `GetParameter` method can be unified if `ValueLookupParameter` do not exists. |
| | 30 | |
| | 31 | * to be continued: `ValueLookupParameter` and `SubScopesLookupParameter` are still waiting for their review |
