Re: [Rails-devel] Starting on a client/server

[Brett L. <wak...@gm...>]

On Mon, 2008-11-03 at 22:07 +0100, Erik Vos wrote:
> > > Perhaps you should create a new branch in CVS for it?
> > >
> > 
> > Once it's in CVS, it can't be deleted. I don't want to check it into
> > CVS only to scrap it entirely or need to completely redesign it.
> > 
> > I'd prefer to keep our CVS tree populated with active code, rather
> > than my random ideas. ;-)
> 
> I'm not familiar with CVS branching, but I thought it would allow us to work
> on different versions in parallel, of course with the ultimate goal of
> merging those versions in a later stage. Never mind.
>  

I think I understand what you're saying. CVS branches are supposed to be
used for working in parallel. To be honest, CVS really sucks at merging.
If you've never had to do it, you should consider yourself lucky. It can
be a really painful process, especially if CVS randomly decides some of
your files are too different, and it just marks the whole file as
conflicting and leaves you to sort out exactly why.

It sounds like you feel strongly that I should bring in my changes now
rather than wait until they are further along. Is that about the size of
it?



> > >> So, you're welcome to check the current state of things here:
> > >> http://github.com/wakko666/18xx/tree/master
> > >
> > > I have downloaded it and tried to run client and server, but
> > > 1. The compiler reports an error in the client:
> > > The method actionPerformed(ActionEvent) of type 
> > NetworkClientWindow must
> > > override a superclass method    net/sourceforge/rails/client/ui
> > > NetworkClientWindow.java        line 77 1225706052046   65163
> > 
> > Hmm...  I'll have to double-check that all of my changes were merged
> > with the last push. For now, removing the @override decorator should
> > remove the issue.
> > 
> > > 2. I can't find a Server main method.
> > >
> > 
> > There is no server main method. Right now, when you hit the "new game"
> > button, it launches both server and client (which currently don't do
> > much of anything). 
> 
> OK, it now works.
> 
> > I haven't yet added in the interface for starting
> > a standalone server. I haven't quite decided what that interface will
> > look like. I'm debating either a command-line interface, or adding a
> > button onto the client start-up interface.
> 
> As I'm currently always running from Eclipse, just a main() method would do.
> Outside an IDE you'll need some kind of a command-line interface anyway.
> Ultimately I would package it in two separate (executable) jars.
>  
> > >> There's some fairly major reorganization that I've done, and I've
> > >> totally refactored the XML parsing.
> > >
> > > Can you elaborate a bit on how you see XML parsing be done your way,
> > > and what the perceived benefits are? What exactly was wrong with it?
> > >
> > > It strikes me that the XMLParser API has the dreaded 
> > Document, Element and
> > > NodeList
> > > types that I have so carefully hidden inside Tag last year...
> > > Will XMLParser replace Tag, or will the former just be used 
> > by the latter?
> > > In any case, GameInfoParser isn't using Tag.
> > 
> > Correct. There's nothing wrong with abstracting away the details of
> > the XML DOM, per se. Tag does a fine job of that.
> > 
> > I don't think our current method is doing anything wrong. Mainly, I
> > wanted to see what it looked like to collect all of the
> > configureFromXML methods into one single parser structure.
> > 
> > The one benefit that this different method adds, is that we don't have
> > to wait until we initialize a specific game object in order to parse
> > the XML for that object. This provides access to that information to
> > things outside of that specific game object. This will avoid similar
> > issues like we had adding in variants and game options.
> 
> The only thing is, that then in some cases the XML must be interpreted to
> know which game-specific XMLParser class must parse some part of it. 

Right. On startup, the only XML that's read is the GamesList. Once we
know which game is being started, we would then read that particular
game's XML, and decide which game-specific parsers are needed.
 
> > At this point, I would consider this experimenting with a different
> > method for code organization. I'm happy to scrap it to retain the
> > existing methods if it doesn't provide any real benefits, or has some
> > obvious problems.
> 
> OK. I'm not too happy with the larger number of classes we will have, but
> I'll drop that comment if definite benefits show up.
> 
> 
> I actually see one potential benefit, although I'm not sure if you will
> judge it as that. Equally well, it might just be one of the goals you are
> aiming for.
> 
> As I think I have explained before: I want to get rid of the many static
> methods in various classes that return non-static and game-dependent
> information. Bank.format() is an obvious point in case. The reason is, that
> when the client/server split is complete, the server should be able to
> accomodate multiple simultaneous games (at least potentially).
> 

I agree with you. Static should be used sparingly, and only when
absolutely necessary.

> These static methods are currently needed to give all kinds of object access
> to properties of "remote" objects: objects that are not directly related to
> it. Ways out of this problem include:
> 
> 1. So far I have been moving towards making all these objects directly or
> indirectly known to GameManager, and passing along the GameManager object to
> all places where such access is needed. I think this is a dead end: there
> are too many different objects around, and putting a GameManager reference
> into all of them looks ugly to me.
> 

Agreed.

> 2. We could try to make all relevant classes a subclass of one common class
> (call it GameObject extends Object), and putting the GameManager reference
> into GameObject. That would fix the problem in one fell swoop, but I'm not
> sure if we wouldn't run into multiple inheritance problems; this need be
> checked, but I think not.
> 
> 3. The common GameManager reference could also be put into XMLParser, and
> then all objects that have an XMLParser companion would have easy access to
> it. But that would leave out those objects that do not parse XML, and I
> think we have some of these as well.
> 
> Actually, option 2 looks best to me, if there are no other top classes
> getting in the way. To be honest, this idea only occurred to me when
> thinking about my previous post in this thread.
> 

I'm not sure I like any of these as individual solutions. 

I can't quite express the problems I see with those solutions in a way
that I'm happy with. I've spent the last 30 minutes trying to type it
out, but none of it sounds right.

In general, I'd like to improve the accessor methods so that we don't
require GameManager to know or do as much.

For example, in Tile there's the isLayableNow() method:

public boolean isLayableNow() {
	return
GameManager.getInstance().getCurrentPhase().isTileColourAllowed(colourName);
}

This method is currently used exactly once in our entire codebase. This
method is used in OperatingRound.

In my mind, there are two problems with this. 

1. Why do we need a whole new method in Tile for information that Tile
doesn't even have?

2. I think that this is information that Tile *should* have. Tile
shouldn't need to call all the way up to the GameManager to figure this
out. 

One likely answer is to change isLayableNow()'s signature to be
'isLayableNow(phaseNumber)' or something similar. That way Tile has the
information it needs to be authoritative for this information, rather
than relying on GameManager to know this. 

My view is that Tile should be constructed with enough information to
know these sorts of things on its own. If it needs GameManager, it's
because we aren't passing in enough information to the Tile objects at
construction time. 

To fix our overuse of static methods, I think we need to improve the
accessor methods and how we're constructing our objects.

When we no longer require instances of GameManager everywhere, then we
no longer require GameManager to have the static getInstance() method. 

I believe the same logic applies to most of the other static methods we
have.

I hope that makes sense.


> > Well, I don't know of any specific things in the game engine that need
> > reorganizing. I know that when we talked about this last, you
> > mentioned that you had wanted to take a different approach on some
> > things.
> 
> I only meant, that I would have started bottom-up by gradually loosening the
> ties between client and server code until we have one class where all
> messages would be channeled through, and then insert the communications
> channel there. Your approach is top-down. Perhaps we need both: yours for
> the framework, mine for the details, but we'll see.
> 

I agree, I think we need both approaches. For the last year or so,
you've been slowly improving the existing code to better support
client/server behavior.

I've just started on the other end of it. ;-)

I think what's important is that perhaps I'm wrong about wanting to wait
to commit this. Perhaps what I've got so far *is* ready for a wider
audience and really should be committed into CVS. I was beginning to
second-guess myself about wanting to wait longer before I committed it
to the project. So, this thread was intended to get your opinions about
it. I figure, if I'm not sure I'm doing it right, I should run it by
someone else. :-)


> Erik.
> 

---Brett.

May all your PUSHes be POPped.