##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.