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

Refactor/enhance Calculator class #26

Open
matyesz opened this issue Jul 6, 2018 · 7 comments
Open

Refactor/enhance Calculator class #26

matyesz opened this issue Jul 6, 2018 · 7 comments

Comments

@matyesz
Copy link
Collaborator

matyesz commented Jul 6, 2018

Calculator is a singleton at the moment, all methods can be used via getInstance. If we can agree that it will be always a singleton, we could change all methods and variables to static. Do a pheasability check first, also check whether in later releases we could you two different calculators paralelly ( in that case it cannot be a singleton anymore).

@phschon
Copy link
Contributor

phschon commented Jul 6, 2018

In my opinion static methods/variables would be a nicer solution, which results in lesser function calls when reading calculator config options and would improve (or rather ease) the overall readability.

Moreover using a private or protected constructor, instantiating this class is prohibited.

@sbondorf
Copy link
Collaborator

sbondorf commented Jul 6, 2018

Are your ideas based on the assumption that Calculator will be (and remain) the top-level class?

I imagine that someday there will be a GUI and one of the valid workflows for experimentation should be to clone the currently visualized network, maybe do some minor adaptation and analyze it with another, potentially different calculator (configuration). All without closing the original calculator.

@matyesz
Copy link
Collaborator Author

matyesz commented Jul 6, 2018

Exactly, if more calculators can existin future then we cannot use a static class, we have to store calculator instances in a dispatcher. For this purpose static approach cannot be used. In that case I would now just remove getinstance and later introduce a dispatcher that can hold several Calculator instances identified by an id.

@sbondorf
Copy link
Collaborator

sbondorf commented Jul 6, 2018

Dispatching to identifiable calculators sound like the most future-proof solution to me.
I think it is worthwhile to keep our code flexible in this way, although I cannot promise that there will be a GUI anytime soon.

@sbondorf
Copy link
Collaborator

sbondorf commented Jul 9, 2018

I tried to remember why there are separate configurations for the calculator and the analysis. That was not always the case, I introduced that a long time ago. The calculator configuration only has/had static member variables whereas the analysis configuration does/did not.

The overall change was motivated by two aspects:

  1. A workflow to automate analysis of one network with different analyses (i.e., analysis of the flow of interest, arrival bounding/s, feature additions like flow prolongation and the burst cap). A workflow probably similar to the one I mentioned above.
  2. Moreover, between those runs in the workflow, we are not allowed to change certain configurations, foremost the Num Backend. That's because we do not have a mechanism to update all existing and still referenced Num objects to the changed implementation in the backend during runtime. Trying to do it leads to crashes ("cannot cast"). Unless we fix that, we should keep the two separate configurations, separated along this line.

@matyesz
Copy link
Collaborator Author

matyesz commented Jul 12, 2018

Did a look. We will also need a builder, to ensure that all mandatory parameters are set before using the class. So the builder will be static and will have a build method to create the instance. Also we need the dispatcher as the container for the Configuration but somehow also need to pass the id of the Configuration. Otherwise we will not be able to reach it from any classes.
If it throws error we have to find that as it is used via interface it should not happen. I would take more time to design this change.

@sbondorf
Copy link
Collaborator

Sure, take the time needed for a good design.

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

No branches or pull requests

3 participants