Free cookie consent management tool by TermsFeed Policy Generator
wiki:ReviewHeuristicLab3.3.0CodeParameters

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.

Last modified 13 years ago Last modified on 03/15/10 16:33:51