Skip to main content
added 1437 characters in body
Source Link
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

##Q&A

When the project continues, the number of components will get quite large, I fear there will be a performance issue if for each request the whole set of components needs to be generated --> should I use LazyLoading?

Since your components are passive objects (as opposed to active object) that get instantiated on demand given the norm, I don't see the need to implement lazy loading here.

Is the ValueService approach good to share data between the components? Is there a better solution?

By using ValueService you have decoupled component logic with the extended state (the value cache) of a project. This is common practice when building state machines.

I still need to implement a specific value-getter, e.g. a user requests the ValueViewModel for a specific key, the components needs to yield an error or warning if not all values were set.

I have addressed possible strategies for conflict resolution in the review. You could extend these strategies to your getter and setters as well.

I think "AbstractComponent" is a better name than "GenericComponent", on a side note.

AbstractComponent is the better name, although it's a typical java name. In C#, it's more common to call this ComponentBase. Some API's would call it ComponentSkeleton.

##Q&A

When the project continues, the number of components will get quite large, I fear there will be a performance issue if for each request the whole set of components needs to be generated --> should I use LazyLoading?

Since your components are passive objects (as opposed to active object) that get instantiated on demand given the norm, I don't see the need to implement lazy loading here.

Is the ValueService approach good to share data between the components? Is there a better solution?

By using ValueService you have decoupled component logic with the extended state (the value cache) of a project. This is common practice when building state machines.

I still need to implement a specific value-getter, e.g. a user requests the ValueViewModel for a specific key, the components needs to yield an error or warning if not all values were set.

I have addressed possible strategies for conflict resolution in the review. You could extend these strategies to your getter and setters as well.

I think "AbstractComponent" is a better name than "GenericComponent", on a side note.

AbstractComponent is the better name, although it's a typical java name. In C#, it's more common to call this ComponentBase. Some API's would call it ComponentSkeleton.

Source Link
dfhwze
  • 14.2k
  • 3
  • 40
  • 101

##Design Choice

  • At first, your API looked like a convoluted pattern to validate user input. However, since you elaborated that validation is highly configurable by country, implies many backend calculations and requires the reuse of intermediate results, I feel your design is justified.
  • I will focus on design decisions that I can't make for you, but I hope you take into consideration for improving your code.

##Data Integrity You allow consumers of the API to directly change the data, bypassing your code. Is this by design or an unwanted leak of data? If this behavior is not wanted, you should either use a different class for storing the values internally, or clone the data both in SetValues and GetValues.

List<ValueViewModel> values = new List<ValueViewModel> {
     new ValueViewModel() {
         Key = "radius",
         Value = 12.0
     }
};
values[0].Value = 0.4;       // <- change data without notifying observers!
project.SetValues(values);   // <- only now, observers get notified

When using ValueViewModel as internal type to store the values, would you allow consumers to change all these properties?

public class ValueViewModel
{
    public string Key { get; set; }     // <- change key, while having a different key in the dictionary
    public object Value { get; set; }   // <- change value, but perhaps the type is invalid
    public bool Valid { get; set; }     // <- override validity, even though there might be an error
    public string Warning { get; set; } // <- are multiple warnings possible?
    public string Error { get; set; }   // <- are multiple errors possible, can the data be valid even if there is an error?
}

You have included convenience methods which enforce some enacapsulation, but the consumer is not forced to use these, and can still bypass them by calling GetValues.

  public void SetError(string key, string error) {
       if (values.ContainsKey(key)) {
             values[key].Error = error;
             values[key].Valid = false;
       }  
  }

##Consistency Consumers can get the values as ValueViewModel by calling GetValues, but observers get a different representation of the data Tuple<string, object>. Your components, being observers, work on both these structures. This introduces unnecessary complexity for consumers of the API.

public IDisposable Subscribe(IObserver<Tuple<string, object>> observer) {
     return new Unsubscriber(observers.Values.ToList()
         .SelectMany(s => s).ToList(), observer);
}

##Interface compatibility A component implements IObserver<T>. This means it can be used in any model that works with observers. Models that call OnCompleted will receive a nice exception. Is there a specific reason to throw an error, or could you just leave it empty as you did with OnError?

public void OnCompleted() { throw new NotImplementedException(); }


##Single Responsibility A project cannot be instantiated with components, instead it requires a norm. A project should not care about the norm, it should care about its components. This would facilitate unit testing. Else you would have to mock FactoryGenerator, while you just need components to work with.

 public Project(string norm) {
        components = FactoryGenerator.GetFactory(norm).GetComponents(valueService);
    }

I would instead make a project factory to create a project given a norm.

public Project(IEnumerable<GenericComponent> components, ValueService valueService) {
    // perform arg checks ..
    this.components = components;
    this.valueService = valueService;
}

public static class ProjectFactory {
    public static Project Create(string norm, ValueService valueService) {
        return new Project(
            FactoryGenerator.GetFactory(norm).GetComponents(valueService)
          , valueService);
    }
}

##Conflict Resolutions Since any component can work with any value, you need a good specification on which components are the owner of which values. What if two components want to register a value using the same key?

  • first one wins: the other uses the value registered by the first
  • last one wins: the first one will use the value overriden by the second
  • error: make clear there is a flaw in the design
public void RegisterValue(string key) {
        if (!values.ContainsKey(key)) {
            values.Add(key, new ValueViewModel() {
                Key = key,
                Value = null
            });
            observers.Add(key, new List<IObserver<Tuple<string, object>>>());
        }  // <- what else ??
    }

I would opt for the first choice. This way, components are independant and flexible. They will only register values that haven't already been registered by another component. There should still be a policy that given a unique key, any component uses the same type and definition for the value.


##General Design ###ValueViewModel

  • Decide whether to use this type internally in your API or you use another type with better encapsulation, maybe even immutable.

###ValueService

  • Be consistent and use only one internal type, both usable by consumers of the API and observers of the values.
  • Determine a strategy for handling conflicts when registering values.

###Component

  • Consider creating an interface IComponent for better testability.
  • Take into account values might already be registered when registering values.
  • Decide which component is responsible for cascading updates of other values when changing a value. radius changed -> change area.
  • Ensure two-way cascaded updates don't result in an infinite loop. What if area changed -> update radius is allowed as well?

###Project

  • Extract configuration logic and move it to a factory.