-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dropping Grid Games: Chess, Go, Checkers #4
Comments
What dependencies does it introduce, that you would want to remove? |
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:
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.
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: Now, with Grid code, this is more complicated since often if tripleA code has the foo call, the grid code can too. 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:
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! 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: Sorry for the long comment, I did trim this back a few times. There is a lot to say, so to summarize:
|
I have several comments:
|
Thanks for taking the time to review and the thoughtful comments. For 1. For 2. 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): 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. 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.
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. |
Fair points. |
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) |
* Rocket fixes * Update serialization * Review feedback plus fix bug with second rocket * Review feedback * Ron's review feedback * Ron's review feedback #2 * Review feedback #3 * Review feedback #4 * Fix rebasing errors * Fix 2 bugs - Infinite not working properly & multiple tech ownership causing accumulation bugs * Update test for new parameter * Fix for retentive grammar rule
* docuStartContribute #1 contribute.md - Initial page "Contribute to TripleA" README.md - Link to new page "Contribute to TripleA" * docuStartContribute #1 contribute.md - Initial page "Contribute to TripleA" README.md - Link to new page "Contribute to TripleA" * docuStartContribute #2 contribute.md - icons try 1 * docuStartContribute #3 contribute.md - icons try2 * docuStartContribute #4 contribute.md - cleanup * docuStartContribute #5 contribute.md - New role: Donor . Map Maker: Skillset * Update contribute.md The Rules Guru profile. I changed "Guru" to "Expert" however 😃 . * Rule Expert link + Capitalization of Roles * Update Forum Moderator See if i did this correctly lol * Update contribute.md Added to Map Maker Skillset * docuStartContribute #6 contribute.md - New roles: Map Admin, Release Tester * Contribute page: TBD for Release Tester + Developer with Merge Rights 🔍 Release Tester - How to become one 🔨 Developer with Merge Rights - How to become one * Update contribute.md See the differences. * Forum Moderator -> Lobby Moderator #2 renaming/linking * ContributePage: Remove *Work in Progress* Co-authored-by: Panther P2 <panther2@users.noreply.github.com> Co-authored-by: beelee1 <timbucktoomany@gmail.com> Co-authored-by: TheDog-GH <71022517+TheDog-GH@users.noreply.github.com>
The grid game code introduces a lot of dependencies into the tripleA code making the tripleA code much harder to work with. This is a big drag on efficiency, and given the features are barely functional and rarely used, I don't think we gain much of anything by having them around (and meanwhile we are committing hara-kiri by keeping this code around).
It's very easy to delete the packages for those three maps. The code update so clients do not freak out about the missing delegates should be mostly straight forward, so this should be feasible and not too much effort.
The text was updated successfully, but these errors were encountered: