tschoppi / starsystem-gen

Automated Generation of a GURPS 4e Star System as described in GURPS: Space
Other
40 stars 16 forks source link

Are private member variables really needed? #48

Open tschoppi opened 8 years ago

tschoppi commented 8 years ago

Introduction

As we refactor the code to be side-effect free (see #47) some weird behavior emerges: Sometimes the member getter is only defined for the parent class, but the member is set in the __init__ of the derived class. The consequence of this is that the member getter tries to return a variable that it no longer can.

Example

An example of the error message is given below:

AttributeError: 'Moon' object has no attribute '_World__averagesurface'

Here the problem is that the method get_average_surface_temp() is defined for the World class, but the __init__ of the Moon class (derived from World) sets the variable self.__averagesurface, which in turn is obfuscated into _Moon__averagesurface and thus no longer available to the getter defined in the parent class.

Discussion

An answer to a question on this topic on StackOverflow.com quite well summarizes the Python way of protection (there is none).

I know that I initially opted for private variables as some of them need to remain constant, as they influence future calculations. On one hand, this was certainly a sensible choice, but on the other hand, we might as well go commando and undo all of the private variable stuff, as the end-user will not be able to change those variables via web interface (and if he can; well, he just got himself a broken star system. Yay him).

tschoppi commented 8 years ago

In 91e7054c7e2ae4e54abc987fc8304b81b75bc740 I converted many private variables to a semi-public state (the one leading underscore indicates that the variable should not be fiddled with) but we need to think about generalizing this completely. This would also eliminate getters and setters (we set all the variables in the initializer) and generally lead to less code, which is a good thing(?)

MyrionPhoenixmoon commented 8 years ago

Yes, less code is generally a good thing, as it makes the project easier to understand and maintain. I have also been thinking about getting rid of the getters and setters, but the setters make sense insofar as we use them in init.

Adding them back in, in case we need them for something in the future is little effort and in general, making our work NOW harder, just for potential future benefit (which I at least can't imagine right now) is not a good decision.

On the other hand, maybe we should move to the single underscore convention and keep the getters, in order to provide an nice interface.

tschoppi commented 8 years ago

We can provide the nice interface by doing great documentation of all the class member variables.

I for one am now slightly confused as to which variable keeps track of what exactly, and lacking documentation comes around to bite me in the back now. I think we should, as with the refactoring for less side effects, refactor the classes to be all-public, with documentation about every variable.