Re: [Rails-devel] Patch for Disclaimer, UserConfirmation and new Main-Class

[Erik V. <eri...@hc...>]

My comments to some of the points raised by Rainer. 

On the code:

It's not easy to read the kind of patch you have sent, 
but from the part on LocalText I understand that you have 
added a dummy constructor and also have factored out getLocalisedText().

To me this is entirely a matter of taste, about which I'm not going to
argue,
but I *would* argue that both should be private or protected rather than
public.

When creating a server (see below) we might want to create an instance
(or a Properties map) per locale, but even then all of this should IMO be
managed internally.


>    - getLogger()
>    Can be also in Util. I put it in here, because it is a 
> rails-spezifig way
> to log. 
>    Find the Logger over a Method makes it easiar to change the fist
> desission to log on package-level.

Sorry, I don't understand at all what you are trying to say here.

>    - VERSION
>    Final variables should be written in upper letters (Sun-Conversion)

I thought that *constants* should be written in upper case.
The question is whether 'version' is really a constant.
One can also see it as a variable that, once assigned, cannot be changed -
but the value is different in each release.

But if you say that this is nitpicking, I agree,
and I have no real problem with making it upper case.

>    We are using 'ea' as a version suffix to show users, that 
> say are using
> an early acces version (if they using the shnapshot)

I would have used 0.x version numbers, but I was too late 
in proposing that to Brett. I don't really mind.

>    - getVersion()
>    Try to avoid direct acess to variables, even if thy are 
> final. That makes
> changes later on easer.

You're right. I was lazy.

> - Config (comment)
>    - Much better not as static class, So you can have userConfig and
> systemConfig. Both should than accessed by RailsSystem or maybe better
> Util-Methods

I was thinking it would be easier to have all (user and system) 
properties in one Properties map, so that the programmer need 
not worry about where a property is exactly defined. 
We could then also easily move properties between property files 
if that would suit us better.

We definitely need static access to properties, otherwise we'll
have to push around another instance reference.
I have no real problem to replace Config.get() by Util.getConfig(),
but I don't see the benefit either. To me, it's just an extra layer
of indirection.

>    - Where do you define the default-values? (if the file is 
> coruppted by
> the user)

Where needed, defaults are specified where the properties are used
when they turn out to be null or empty. 

Not every property has an explicit and fixed default value.
The default money amount format is game-dependent.
The default language is (British) English, because the default
resource bundle is written in that language.
The default log4j properties are (by definition) empty.
Etc.
  
> - CodeStyle (comment)
>    - public and protected variables: You are much to 
> broadminded with it.
> You should capsulate as much as possible, using getter- and 
> setter-methods
> for access.

One problem I have with Java is, that you cannot distinguish 
between variables that may only be visible to subclasses,
and variables that may be visible to all classes within a package.
Both are "protected". 

In my code in this project, most or all "protected" variables are meant 
to be used in subclasses only, even where such subclasses do not yet exist.
We expect that a lot of subclassing will be needed in the future
when we are going to add dissimilar games. 
So I am only prefixing instance variables with "private"
where I am pretty sure that these will never be used in subclasses.

The rule I use, is that subclasses have free access to its superclass
variables without need to use a getter. But non-related classes
in the same package should always use getters and setters.

BTW I think that the 'game' package is getting
too large, and that we might need to split it up.
That would have the side effect of better "protection" too.

I agree that in principle there should not be public variables.
Do we really have sinned as much as you seem to imply?
It is possible that I have not always been consistent,
so I would appreciate your examples of wrong use of public etc.

And (hmmm) see my comments on your version of LocalText above....

>    - static: Much to broadminded also. You should avoid 
> static as much as
> possible. (static constants are a break in the rule)

As a matter of principle? I don't see why.
To me, this is very project-dependent.

>    You will get a lot of trouble to bring this code running in a
> server-environment. No chance. 
>    Even singletons are not very good on servers (but easy to 
> transform).
> RailsSystem as a geneneral System-API-Class and Util for general
> transformation and methods should be the only Classes which 
> have static
> methods which are not factory methods or system-wide-parameters
>    Hard words but I have a bad feeling for the time this 
> project groose more
> complex and it will be nearly imposible to have a 
> server-side-version with
> multible games with this code.
>    Start smart is a good thing, but some day, you have to 
> redesign it and if
> you go this way  farther it will be imposible someday - sorry.

I am fully aware that a lot of work needs be done 
to make a multi-game server out of this code.
If we ever get that far.

The problem is, that if you don't have class methods,
you need at least some reference variables in many parts 
of the code to access widely used instances.

Several parts of the code need to be able to find out in which turn, 
round or phase the game is (this all may have changed
any moment, e.g. while processing a train buy).
So there must be some central point of inquiry about all
aspects of the game state.

I am fully aware that the mixed use of class and instance variables
and methods is a mess in some places (e.g. Game/GameManager),
and that this needs to be redone in a better way.
However, in my opinion the code is not yet in a sufficiently
mature state that we can take decisions on exactly how to do that.

I suppose the best way will be to make sure that every 
instance that must take decisions has a reference to an
instance of either Game or GameManager, and can find out
the current status by calling that instance's methods.

I don't think this will be very difficult to do. 
So far I did not have the feeling that this is more urgent
than improving the code structure in more fundamental ways,
to enable undo/redo and to prepare a client/server split,
which is what I have been doing the last few months, and still do.


Aside: I sometimes take shortcuts to use the little bits of time
I have as productively as possible.
And quite often I come back (much) later to that code
and fix it, or improve it in some other way, or entirely
remove it or replace it with something better.
There is quite some code that I know must be improved, 
or replaced, or whatever -  but often I just have to postpone
my thinking on what exactly needs be done.
In my experience, things usually become clear when the time is ripe.

>    I know, my code is not a good example how it shoud be 
> (since it is not
> much code at all and most of it in RailsSystem - the 
> exception of the rule, 
>    Maybe I can refactor LocalText or Config to show you what 
> I mean, if you
> want it.

Showcases are always welcome to me.
It is from code from other people that I learn, 
even if I adopt nothing or just a little of their style.
I'm sure it's the same with you.

Sorry that this reaction has become so long, 
but I hope I have been able to clarify a few aspects.

Erik.