3
\$\begingroup\$

I have C# ASP.Net Core Webapi application that makes some calculations for physical models. I have this project class:

public class Project {
    public IEnumerable<GenericComponent> components;
    private ValueService valueService = new ValueService();

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

    public IEnumerable<ValueViewModel> GetResults() {
        return valueService.GetResults();
    }

    public void SetValues(IEnumerable<ValueViewModel> values) {
        values.ToList().ForEach(t => valueService.SetValue(t.Key, t.Value));
    }

    public bool Valid(string componentName) {
        return components.Where(s => s.Title == componentName).Single().Valid();
    }
}

That creates smaller components that do some calculations. Depending on the norm parameter, another set of components is created (via the factory). Each component has a list of variables that may exist (the frontend know which ones) and it can do some calculations on those variables and on variables that are being initialized in other components (with 3 example implementations of GenericComponent).

public abstract class GenericComponent : IObserver<Tuple<string, object>> {
    public readonly IValueService valueService;
    protected IDisposable cancellation;
    public List<string> variables;
    public List<string> observingVariables;

    public GenericComponent(IValueService _valueService) {
        valueService = _valueService;
    }
    public string Title { get; set; }
    public void OnCompleted() { throw new NotImplementedException();   }
    public void OnError(Exception error) {// no implementation
    }
    public abstract void OnNext(Tuple<string, object> value); 
    public abstract bool Valid();
}

public class LoadComponent : GenericComponent {     
    public LoadComponent(IValueService valueService) : base(valueService) {
        variables = new List<string>() { "load" };
        variables.ForEach(s => valueService.RegisterValue(s));
        cancellation = valueService.Subscribe(this, variables);
    }
    public override bool Valid() => valueService.GetValue("load")?.Valid ?? false;
    public override void OnNext(Tuple<string, object> value) {
        double load = (double)value.Item2;
        if (Math.Abs(load) < 1) {
            valueService.SetWarning(value.Item1, "Small load, should be larger");
        }
    }
}


public class ValidationComponent : GenericComponent {

    public ValidationComponent(IValueService valueService) : base(valueService) {
        variables = new List<string>() { "validation.load" };
        variables.ForEach(s => valueService.RegisterValue(s));
        observingVariables = new List<string>() { "load", "area" };
        cancellation = valueService.Subscribe(this, observingVariables);
    }
    private double CalcMargin() {
        return 0.5 * (double)valueService.GetValue("area").Value 
            - (double)valueService.GetValue("load").Value;
    }
    public override bool Valid() => CalcMargin() > 0;
    public override void OnNext(Tuple<string, object> value) {
        if (valueService.GetValue("load").Value != null &&
            valueService.GetValue("area").Value != null) {
            double margin = CalcMargin();
            bool valid = margin > 0;
            valueService.SetValue("validation.load", "margin is: " + margin.ToString(), 
                valid);
            if (margin < 1) {
                valueService.SetWarning("validation.load", "Very tight margin");
            }
        }
    }
}

public class CrossSectionComponent : GenericComponent {
    public CrossSectionComponent(IValueService valueService) : base(valueService) {
        variables = new List<string>() { "area", "radius" };
        variables.ForEach(s => valueService.RegisterValue(s));
        cancellation = valueService.Subscribe(this, variables);
    }
    public override bool Valid() => valueService.GetValue("radius")?.Valid ?? false;
    public override void OnNext(Tuple<string, object> value) {
        if (value.Item1 == "radius") {
            double radius = (double)value.Item2;
            if (radius < 0) {
                valueService.SetError("radius", "Radius is must be larger than 0");
            } else if (radius < 1) {
                valueService.SetWarning("radius", "Radius is very small");
            }
            valueService.SetValid("radius", true);
            valueService.SetValue("area", radius * radius * Math.PI, true);
        }
    }
}

Each component has a list of values with keys that are unique over the entire model (here I probably need more checks, whether truly all keys are unique). At the moment I use a ValueService, where each component registers its values with the respective keys. The ValueService can be used to get values from other components in order to do validation calculations or intermediate steps.

public class ValueService : IValueService {

    private IDictionary<string, ValueViewModel> values = new Dictionary<string, ValueViewModel>();
    private IDictionary<string, List<IObserver<Tuple<string, object>>>> observers = 
        new Dictionary<string, List<IObserver<Tuple<string, object>>>>();

    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>>>());
        }
    }

    public IEnumerable<ValueViewModel> GetResults() {
        return values.Where(s => s.Value.Value != null).Select(s => s.Value);
    }

    public void SetValue(string key, object value) {
        if (values.ContainsKey(key)) {
            values[key].Value = value;
            observers[key].ForEach(t => {
                t.OnNext(new Tuple<string, object>(key, value));
            });
        }
    }

    public void SetValue(string key, object value, bool status) {
        SetValue(key, value);
        SetValid(key, status);
    }

    public void SetValid(string key, bool status) {
        if (values.ContainsKey(key)) {
            values[key].Valid = status;
        }
    }

    public ValueViewModel GetValue(string key) {
        if (values.ContainsKey(key))
            return values[key];
        return null;
    }

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

    public IDisposable Subscribe(IObserver<Tuple<string, object>> observer, IEnumerable<string> keys) {
        keys.ToList().ForEach(key => {
            if (!observers.ContainsKey(key))
                observers.Add(key, new List<IObserver<Tuple<string, object>>>());
            if (!observers[key].Contains(observer))
                observers[key].Add(observer);
        });
        return new Unsubscriber(observers.Values.ToList().SelectMany(s => s).ToList(), observer);
    }

    public void SetWarning(string key, string warning) {
        if (values.ContainsKey(key)) {
            values[key].Warning = warning;
            values[key].Valid = true;
        }
    }

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

}

The ValueViewModel is used to communicate with the front end

public class ValueViewModel
{
    public string Key { get; set; }
    public object Value { get; set; }
    public bool Valid { get; set; }
    public string Warning { get; set; }
    public string Error { get; set; }
}

This is part of a stateless API, i.e. for each HTTP request (at the moment) a new Project object is generated and used to make validations (via an array of ValueViewModels).

My API controller looks like this:

    public IActionResult postData([FromRoute] string norm, [FromRoute] string component, [FromBody] ValueViewModel[] data)
    {
        Project project = new Project(norm);
        project.SetValues(data);
        return Ok(project.GetResults()); // <<-- previously mistake, it should be GetResults, that returns a ValueViewModel with populated values (where possible)
    }

In order to make some local tests, I created the following console application

class Program {
    static void Main(string[] args) {
        Console.WriteLine("Hello world");
        Project project = new Project("Eurocode_CH");
        List<ValueViewModel> values = new List<ValueViewModel> {
            new ValueViewModel() {
                Key = "radius",
                Value = 12.0
            }
        };
        project.SetValues(values);
      Console.WriteLine(Newtonsoft.Json.JsonConvert.SerializeObject(project.GetResults()));
        values[0].Value = -0.4;
        project.SetValues(values);
      Console.WriteLine(Newtonsoft.Json.JsonConvert.SerializeObject(project.GetResults()));
        values[0].Value = 0.4;
        project.SetValues(values);
      Console.WriteLine(Newtonsoft.Json.JsonConvert.SerializeObject(project.GetResults()));
        values.Add(new ValueViewModel() {
            Key = "load",
            Value = 1.2
        });
        project.SetValues(values);
        Console.WriteLine(Newtonsoft.Json.JsonConvert.SerializeObject(project.GetResults()));
    }
}

This is the output

[{"Key":"area","Value":452.38934211693021,"Valid":true,"Warning":null,"Error":null},{"Key":"radius","Value":12.0,"Valid":true,"Warning":null,"Error":null}]

[{"Key":"area","Value":0.50265482457436694,"Valid":true,"Warning":null,"Error":null},{"Key":"radius","Value":-0.4,"Valid":true,"Warning":null,"Error":"Radius is must be larger than 0"}]

[{"Key":"area","Value":0.50265482457436694,"Valid":true,"Warning":null,"Error":null},{"Key":"radius","Value":0.4,"Valid":true,"Warning":"Radius is very small","Error":"Radius is must be larger than 0"}]

[{"Key":"area","Value":0.50265482457436694,"Valid":true,"Warning":null,"Error":null},{"Key":"radius","Value":0.4,"Valid":true,"Warning":"Radius is very small","Error":"Radius is must be larger than 0"},{"Key":"load","Value":1.2,"Valid":false,"Warning":null,"Error":null}]

My concerns are

  • 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?
  • Is the ValueService approach good to share data between the components? Is there a better solution?
  • 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 think "AbstractComponent" is a better name than "GenericComponent", on a side note.
\$\endgroup\$
  • \$\begingroup\$ You take the view models as input, perform manipulations on these objects, and then return a boolean. Are there other operations in your API that allow to send the view models back over HTTP? \$\endgroup\$ – dfhwze Jun 8 at 12:53
  • \$\begingroup\$ 'getresults' is one Im sending back. The frontend loops through all items and populates the from accordingly. What boolean do you mean? \$\endgroup\$ – rst Jun 8 at 13:03
  • \$\begingroup\$ return Ok(project.Valid(component)); // <- you return a boolean here and I was wondering whether all your operations would return the boolean (is valid) orsometimes also the getresults. Because atm you only have Post implemented. \$\endgroup\$ – dfhwze Jun 8 at 13:06
  • \$\begingroup\$ What's the difference between norms - do they return entirely different components, or the same components but with a different configuration (such as different load/radius thresholds)? Also, the example could be simplified to a single function that takes load and radius parameters. What problem does having a component system solve? \$\endgroup\$ – Pieter Witvoet Jun 8 at 13:24
  • \$\begingroup\$ @dfhwze That was wrong, sorry, I copied the false Post request. The main purpose is of course the GetResult method that returns a more populated ValueViewModel. The Valid method can be used further in the project, where you want to use single validations for certain components (mainly in the project object) \$\endgroup\$ – rst Jun 8 at 15:55
2
\$\begingroup\$

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.

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.

\$\endgroup\$
  • \$\begingroup\$ Wow, that really is extensive! On Consistency: I agree, I really don't like the Tuple approach but I needed to tell an observer which OnNext value I'm actually passing. Passing the whole ValueViewModel appears to be an overkill although an IValueVM would be better with different implementations for different use-cases. Interface compatibility: that part is just missing, you're right. \$\endgroup\$ – rst Jun 8 at 19:37
  • \$\begingroup\$ @rst I'm not sure it's overkill, since I see that some of your components call ValueService for the ValueViewModel in OnNext. So they need the full instance, rather than just the value in some occasions. \$\endgroup\$ – dfhwze Jun 8 at 19:40
  • \$\begingroup\$ The part with the ProjectFactory is good, that should be implemented. On the part with Conflict Resolutions, I will make options 3, it should be clear there is a flaw. Component: Issue 3 and 4 are actually tough ones! \$\endgroup\$ – rst Jun 8 at 19:44
  • \$\begingroup\$ @rst I agree with issue 3 and 4 being both tough design decisions and technically challenging. One thing you can do is to only update a value if it differs from the current value. This way, whether radius or area gets changed first, results in updating the other, results in a requested update of self. But because the value does not differ, the flow short-circuits, avoiding an infinite loop. \$\endgroup\$ – dfhwze Jun 8 at 19:47
  • \$\begingroup\$ I do lots of Angular coding (for my taste) and there there are many "AbstractForms" e.g., ComponentBase sounds better than ComponentSkeleton. \$\endgroup\$ – rst Jun 8 at 19:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.