Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NetworkEventsManager is tightly coupled with ConfigurationData #7

Open
mpostol opened this issue Nov 20, 2019 · 4 comments
Open

NetworkEventsManager is tightly coupled with ConfigurationData #7

mpostol opened this issue Nov 20, 2019 · 4 comments
Assignees
Labels
Aggregating Master issue

Comments

@mpostol
Copy link
Collaborator

mpostol commented Nov 20, 2019

This approach is not compliant with the architecture proposed in:

Reactive Networking Application Architecture

It has very serious consequences:

  1. It is against the separation of concerns paradigm
  2. Configuration cannot be changed separately - it is very difficult to analyze the impact scope of these changes.
    You said Unfortunately the library does not provide any easy way to map data onto properties which will be later used for displaying the data to the user. - my concern is if it is true. RferenceApplication contains an example of how to do it.

You said: For this reason I had to build some sort of proxy which will allow to manage the process much more easily. - my concern is how to prove it, how to measure the effort required to (not sure) to implement or to understand or both.

@mpostol mpostol added the question Further information is requested label Nov 20, 2019
@Drutol
Copy link
Owner

Drutol commented Nov 26, 2019

It is against the separation of concerns paradigm

At the time of writing this code the assumption was that configuration is static and does not change, hence it's treated as a part of application domain. That being said it's only being used for accessing extra pieces of metadata about given instance(repository) so removing it won't have much of an impact given that in the end it should come separately after obtaining information about given repository from external sources.

Now that I think of it there will be 3 kinds of data to discover:

  1. How to decode packets.
  2. What is the meaning of the decoded data for a whole type.
  3. Specific data about the instance.

While the first two should be contained in one configuration file the third one is separate piece of data.

Configuration cannot be changed separately - it is very difficult to analyze the impact scope of these changes.

Not quite sure what you mean, separately from what? Reply to the first point may be duplicate of this.

Unfortunately the library does not provide any easy way to map data onto properties which will be later used for displaying the data to the user.

What I meant here is from pure library user standpoint, right now we have this "fountain" of data in form of IConsumerBinding interfaces. What I'm doing here is just putting next layer on top of it so that I as a user can define class which will be automatically mapped to these IConsumerBinding instances so that I don't have to do the plumbing every time I add a new model. With this tooling I can either easily create a class with:

[ProcessVariable] public double PipeX001_FTX001_Output { get; private set; }

or build one in runtime:

Properties = new Dictionary<string, BuiltInType>
{
    { "DrumX001_LIX001_Output", BuiltInType.Double},
    { "PipeX002_FTX002_Output", BuiltInType.Double}
}

and be done with it. It's just a "quality of life" feature.

@mpostol
Copy link
Collaborator Author

mpostol commented Nov 27, 2019

Thanks for comments. Let me distinguish two independent topics related to this part of code:

  1. IConfigurationDataFactory implementation
  2. IBindingFactory implementation

I will add comments to these topics separately.

@mpostol
Copy link
Collaborator Author

mpostol commented Nov 27, 2019

IConfigurationDataFactory

  • Static doesn't mean injected. The idea behind this interface implementation is late binding - to allow modification after deploying the main library. It requires loosely coupled parts, but now Configuration is tightly coupled in your code and Reference application. Unfortunately, we don't have proof of the concept that dependency injection is possible in this respect.
  • The Configuration library depends on Logger, so it must use any functionality to inject the logger part. Now it uses ServiceLocator, but because it is tightly coupled we can use constructors to provide a logger.
  • Part of your code may be recognized as a clone of the functionality provided by the Configuration library, i.e. serialization. The library provides the same functionality offering DataContractSerializer and JsonConvert. Visit the ReferenceApplication and Configuration library to get more.
  • You have correctly applied the possibility to expand the base configuration by adding your stuff but using both serialization methods at the same time may be recognized as a violation of the best practice rules, this way we have >>16 versioning possibilities of the serialization stuff.

Try to improve your code and define separate issues targeting anyone you will consider.

Dynamic configuration coupled with discovery functionality is an independent topic that we must discuss after collecting all ideas targeting this topic.

Once more thanks for the comments. I will use them to improve the documentation. The work targeting this issue mpostol/OPC-UA-OOI#401 is conducted on the branch Configuration

@mpostol
Copy link
Collaborator Author

mpostol commented Dec 3, 2019

I will review the modification and report any problems in separate issues if needed.
Consider this as an aggregating one. We will close it after closing all dependent issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aggregating Master issue
Projects
None yet
Development

No branches or pull requests

2 participants