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.
- 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.
- 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.
Download in other formats:
