Remove magic from _ShockFactory #15

Open
Juanlu001 opened this Issue Dec 9, 2013 · 6 comments

Projects

None yet

2 participants

@Juanlu001
Member

Right now the code of _ShockFactory may be smart but it's full of black magic. Very difficult to follow, would be better if more explicit.

@Gillu13
Contributor
Gillu13 commented Jun 9, 2014

hello Juan,

Tell me if I'm wrong but I understand this piece of code like this:

One can define a Shock object by using various combinations of entry arguments: ['X', 'beta', 'gamma'] or ['X', 'theta', 'weak', 'gamma'], X being either M_1, M_2 or p1_p2 (if i'm not wrong again, M_1 is the only implemented one so far).

The _ShockClass's constructor however takes only a certain combination of arguments: ['M_1', 'beta', 'gamma']. Thus, one needs intermediary private functions (_ShockFactory() and _from_deflection_angle()) in order to convert input arguments into the accepted combination and eventually construct the shock wave object.

I've silly and naive questions: why not simply including in _ShockClass.__init__() the task of dealing with **kwargs? Or, if an explicit argmuments management is needed, why not creating a basic Shock class with common properties and several subclasses that inherit properties from Shock but have different constructors (depending on known parameters)? Like this it would be possible to have a NormalShock class, remaining consistent with the nozzles module for instance, which uses the NormalShock class .

@Juanlu001
Member

Excuse me Gilles, you caught me in exams time and I have some
constraints but I saw this and I'll give you an answer as soon as I can
:) In the meanwhile, the two offending commits are 49d1379 and 6a8a7ad.

@Gillu13
Contributor
Gillu13 commented Jun 11, 2014

hello Juan,

Don't worry, there is no rush. Good luck for your exams! In the meanwhile, I'll have a look at those commits.

@Gillu13
Contributor
Gillu13 commented Mar 26, 2015

Hello Juan,

I am wondering if you are still working on this code? Does it worth commiting improvements?

Regards,

Gilles

@Juanlu001
Member

Hello @Gillu13, thanks for your interest. The truth is that it's been a long time since I last worked on this, but I am not sure if I will ever do again or not. I am in the process of teaching Python to some friends (cc @AeroPython/isa) and we will have to decide what to do with this code.

My suggestion is that, if you are interested in developing this, you commit your changes in your fork as you desire. In some weeks or months we can decide the future of the project.

Best regards and thanks again 😄

@Gillu13
Contributor
Gillu13 commented Mar 27, 2015

Hello Juan,

thanks for your answer. OK, let's do this.

Best regards.

Gilles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment