Dropping Grid Games: Chess, Go, Checkers about triplea HOT 6 CLOSED

What dependencies does it introduce, that you would want to remove?
Most of the abstraction needed to make these games possible is both good, and already existed before any of these games were added.
I am against removing these games unless a very compelling argument can be made.

from triplea.

I think the reasons to remove these games is pretty compelling. Or at least from the point of view of the tripleA code base. I'd suggest that these games would be better home'd in a forked repository.

Dependencies - I mean this in terms of code coupling. For example, a lot of Grid code calls library code in the TripleA code base. If an API in that library code can be simplified, then suddenly we have to ensure we support and also update the Grid code. I think our focus should be on the tripleA code, that is where we get value in this project.

I disagree about most of the abstractions being good. In my experience a lot of them have been copy paste with heavy reliance on inheritance. In fact, it's these abstractions that I want to fix in the tripleA code.

So let me back up a bit. If we keep the grid code, we've got some work to do:

1. we should fix the copy paste in that code and improve the general quality
2. we need to add testing for it
3. we need to finish the maps and get them to really be playable.
4. support the maps going down the road, perhaps grow to more maps
5. finish kingstable (is this another game, a modification of checkers/go/chess?)

Given that this project is constrained to two developers, and seems like the hay day of development is past, I don't think we'll ever get to those tasks. Any new developers are likely to pick these tasks up as a greenfield project (meaning, if someone new works on it, they would rewrite it, or probably experiment writing it in some new language or something, people are generally reluctant to go into old code). I recall seeing the Chess code a half decade ago, given that much time, seems unlikely that will change.

Okay, so keeping this stuff around gains us not very much. But it also drags, actively.

1. We're having to spend time on this stuff at all, talking about it, looking through the code, if it were gone we would save all this time. For example, adding @OverRide tags to this code expanded the size of the code review. So this is just more stuff to maintain, it's just more work for us whenever we want to change stuff.
2. There is a lot of coupling to the tripleA code that makes the tripleA code harder to work with. Now I regret we don't have the triplea-test issue board, I had a good write up on this there :/ Anyways, let's take a few examples. We have methods sometimes that look like this:
   foo( this, null, null, false, true, false);

Often in these cases the parameters will always be called with the same value, may not even be used, or we can apply other refactoring magic to reduce the call very easily to:
foo( this );

Now, with Grid code, this is more complicated since often if tripleA code has the foo call, the grid code can too.
Case #1 - Grid code uses same arguments, so we can refactor to "foo( this )"
Case #2 - Arguments vary: Need to now understand the grid code and maintain the functionality, also means the tripleA code is now more complicated than it needs to be, the refactor looks like this for example now:
tripleA: foo( this, true, false) ;
Grid: foo( this, false, true);

So, at this point. Let me give some better context. I'm not a huge fan of walking in and suggesting we rip out a lot of stuff, particularly some that has some functionality and is cool stuff too (I coded up a Go game last summer with the intention of looking at Go AIs, so this is stuff I'm really interested in). But, from what I can see, one of the priorities of this code base is improving quality and test. To do that, removing 20% of the code off the bat makes that job much easier. Particularly when this code is not actually complete, and nobody plays these maps on the libraries. I'd suggest nobody probably ever will either:

1. This code is 5+ years old, that's evidence I think it won't be finished, hasn't been a need to finish it now, probably won't be one anytime soon
2. The priority of this code base can easily be agreed upon to not finish the grid games, it would be to do other stuff
3. Nobody plays these maps in the lobby, nobody is going to ask for it too loudly
4. @veqryn , TBH I think you would now be the only person in the world that would truly be interested in finishing this code and supporting it. Anyone else I think would probably prefer a green field project. So ask yourself a bit if you really want to spend a few weeks on this stuff and get it up and running.

Sorry for the novel, there is a lot to be said about why it's good to move this code out of the tripleA code base. Let me give an example of working with the TripleA code recently, trying to test TripleAPlayer.java:

TripleAPlayer extends AbstractHumanPlayer which extends AbstractPlayer

I wanted to test TripleAPlayer. Looking at the first method, "start", I asked, what does it do? Basically it is a void return method, so difficult to test, and makes a bunch of calls to an object called "m_ui". So okay great, to test this I simply need to overload the constructor of TripleAPlayer with that object so I can then mock it. But it turns out m_ui comes from AbstractHumanPlayer. Also, TripleAPlayer is not at all functional until "setUI" has been called on AbstractHumanPlayer. Meaning something somewhere is going to create these objects, keep them around completely non-functional (NPE if you call any method), until someone somewhere calls setUI in which case the player objects are now initialized.
But getting back to m_ui, it being in the parent class is a problem. Setting the mock object via a parent class is weird, so it'll be hard to understand for future developers. Secondly, it adds an extra layer to the unit test, makes it brittle, if we changed AbstractHumanPlayer we could also break tests for TripleAPlayer which should be unrelated.

But! There is a way out of this, there is a way to test TripleAPlayer!! AbstractHumanPlayer is extended by two classes, TripleAPlayer and GridPlayer. GridPlayer looks like it was a copy paste of TripleAPlayer with 40% of the code removed and some small pieces modified. The easy way to fix things would be to delete the GridPlayer, in which case the one variable and the one setter method in AbstractHumanPlayer can slide into TripleAPlayer.

Though, if we keep Grid player code around, then to test TripleAPlayer the m_ui object and the one setter method will probably be copy pasted into bother TripleAPlayer and GridPlayer so we can get rid of the shared variable, and AbstractHumanPlayer likely turned into an interface with the common methods between the two so we can get rid of the "instanceof" checks for code that is like this:
if( player instanceof TripleAPlayer.class)
ui = TripleAFrame<>();
else if( player instance of GridPlayer.class)
ui = GridFrame<>();

Sorry for the long comment, I did trim this back a few times. There is a lot to say, so to summarize:

• there is not much good reason to keep the grid code, nobody plays it in the lobby
• extra code is extra complexity. Abstractions done for this code are maybe clean in some instances, but largely they are not and would need work. This represents extra work.
• complexity is perhaps enemy number one for the triplea code base right now, reducing this is a priority
• keeping this code around is slowing us down. We don't have the capacity to support it, nor frankly the mandate, nobody is asking for it AFAIK. There are other really good alternatives for chess/checkers/go available, but not at all for the other tripleA maps we host. I'd personally much rather us spend time working on 270BC than chess/checkers. Meanwhile core engine work suffers because of coupling between tripleA and grid code.

from triplea.

I have several comments:

1. Your example is the TripleAPlayer class, which looked like the crap you see since long before the Grid related stuff was ever added (ie: previous to 2013). I hate this class and how it works, but it is not GridPlayer's fault that it, and its parents, are so badly written. Proper abstraction of it could be done, for example so that m_ui is set in the constructor of whichever level it exists in, and get rid of the code duplication and instanceof checks, etc.
2. I am fine to get rid of kingstable, the puzzle games. But the grid games provide a counter point so ****bro's claims about TripleA and A&A. TripleA is not A&A, partially because we have the grid games.
3. There have been no bug reports on stuff related to Chess/Checkers/Go, and in my process of creating them, ended up finding and fixing lots of other bugs that already existed in the engine, that probably would not have been found without that abstraction. My personal experience in doing this and similar abstractions, and seeing the ways that the abstraction simplified things or found & resolved bugs, have led me to prefer some level of abstraction, over hard coded classes.
4. AFAIK their existence is not slowing anyone down. They have not caused any bugs, and noone has to tip-toe around them to get our planned features implemented, because they are not related to those features in any way.
   The only way they could be slowing someone down is if that person was planning on refactoring certain critical low level core engine components. There are no features we need, that require a refactor of core engine components, so this would be purely done out of principle. This is quite a huge undertaking, and I'm curious if you would stay on for the full duration, seeing it through both to completion from a code point of view and also to a nearly bug free state from our user's perspective (or at least as bug free as it is right now)(ie: staying around for the maintenance and bug fixing iterations months/years after the fact).
   I've received lots of comments and feedback from people in the past, saying to use such-and-such framework/testing/utility/etc. Pull out all the network code and redo it as this. Refactor that. If only you used XXX all your "problems" would be solved. They disappear after a few weeks, leaving me with all the baggage.
   So instead, I take the approach of incremental changes and refactoring, on an as needed basis. If the purchasing code and functions are causing bugs, or it needs a new feature added to it, then I take the opportunity to refactor it as part of fixing it or adding to it.

from triplea.

Thanks for taking the time to review and the thoughtful comments.

For 1.
Turns out adding m_ui to the constructor of TripleAPlayer is quite difficult, there is quite a lot that happens between the creation of the object and when m_ui is injected into TripleAPlayer. I believe IIRC Grid makes this yet a bit more tricky to do. Grid comes into play since fixing the structure in TripleAPlayer you need to also fix/modify/preserve what was built on top of it.

For 2.
First, I'm glad we can easily agree on the puzzle and kings table game being moved out of repo. On the other point, I know hasbro is a sore point. I was playing the game at the time they sued and things got shut down, everyone was worried TripleA was done.

Hasbro happily I believe can be reasoned to not be a consideration when removing code. If for argument we were in copyright violation by removing code, then that would mean we have hasbro copyrighted work plus other work. If that were the case, we were would be in infringement today. Unless we add stuff, or unless we are already in copyright infringement, there should be no way you can infringe on someone else copyright by removing stuff.

I went back to the thread that spun up around the time, there is some commentary in there that makes me feel comfortable not even saying we need a lawyer to weigh in on this (and also happily this also clarifies what we need to do if say we want to add a risk-like map):
"Copyright protects only the particular manner of an author’s expression in literary, artistic, or musical form." "They can't protect the "Game" itself" http://triplea.sourceforge.net/mywiki/Forum#nabble-td3279250

For 3, no bugs and finding bugs. I don't know if grid games have been played as much as tripleA games, in which case it hasn't had the user base or real world usage to generate many bug reports. I'm not sure how many really realize those games are there or have them downloaded.
With respect to finding bugs, going through this code base in detail I think can quickly find bugs, so I would suggest some of the benefit of grid was simply having to work with the code and get it to be reasonable. On the other hand, I do not have full context and honestly do not know the ratio of factors that were involved, so Grid may have been a healthy exercise. Though, it having helped to add it in is not necessarily good reason to keep it. Removing it will not re-introduce bad structure or complexity, and will help us clean up what is left.

For 4, you've expressed some valid concerns. I've encountered similar in my past before. I'll start I suppose by the refactoring because of principle. The principle I believe is that namely dirty code is buggy, labor intensive, and slows down future efforts. Thus the effort to refactor bits and pieces is to allow future efforts to move along faster. I'd view this as a good thing, if you see actual code changes that are good, it simply means you have someone that wants to work on the code base. There is a balance to be struck of course though, really several balances.

• one balance is being incremental. For this I see this project as missing some really big things. There are many different options for those big components, but we lack all of them. For this deciding which to use eventually is helpful so we can start forming a picture of the bigger system and where we are going. Hence the 100 million topics I have raised. This project until a week ago only had a mostly automated way to build a JAR and run tests (and the tests were broken). Let me say it another way, this is an old project, which is interesting, though there is a lot to do to update it so we can do more.
• features - if developers or anyone willing to work with tripleA leaves after a short bit, they aren't going to generate new features. I'm motivated since I play the game... Also in my defense, I haven't necessarily proposed we do anything that requires a global refactor, mainly directions and incremental stuff. I've been cleaning some stuff in large part as a way to understand some of the code. Also for the sake of being able to work with it at all.

I don't think there is too much disagreement really. I would say that core engine features do need modification for most anything. The source forge ticket queue is very long, quite a few items were game rules. Modifying those items do require core game changes.

Last, I am also looking at adding in Risk rules and a map, so that has had me looking at the core engine quite closely! That is a large undertaking though, but would be really fun to do : )

I'd like to summarize I think. A valid concern is that Hasbro would have issue if the Grid games were to disappear. With some reasoning, it's about impossible for us to infringe copyright by removing stuff. The other concern is that it only gets done partially. To that I can't say much other than you'll have chances to look at pull requests and gate keep the code in that manner. If it's any relief the rule 1 for developers is to not break stuff, and playing the game, and given the community being on a tipping point for growing or shrinking, I've no incentive to let stuff be broken.

from triplea.

Fair points.
Lets remove puzzle stuff for now, but keep grid (and kings table) for now.
I'd like to see what else happens around this project, and your code pulls for other unrelated things, before I want to make any decision about grid games.

from triplea.

Sounds like plan. Thanks for the consideration and lengthy discussion. Closing this issue, no reason to leave it open while we wait around (and execution of the removal can very easily get a new issue created for it)

from triplea.