Feature ChoiceManager refactor

fredg1

Member
Not necessarily, but arbitrarily splitting it in this fashion isn't a clear improvement.
Any insight into how we should?

I would really like to insist that whichever way we use should preserve the order of the choice adventures (i.e. avoiding "putting all choice adventures related to the elemental charters in this file, putting all choice adventures related to dreadsylvania in that one").
Doing so allows us to find "missing" choice adventures more easily, as well as duplicates (though there's the test for that one).

While doing the whole refactor on my end, using this format did in fact allow me to find a number of missing choice adventures (like one of the ChibiBuddy(tm) adventures), by noticing a gap in the choice adventures, what is before and around it, and digging further. I'd submit them, but at that point, my version is so different from ChoiceManager that it's too hard to tell what I changed.

Also, the splitting isn't actually "arbitrary": it's based on the splitting of choice adventures from the kolwiki's Choice Adventures by Number page.
 

MCroft

Developer
Staff member
Any insight into how we should?

I would really like to insist that whichever way we use should preserve the order of the choice adventures (i.e. avoiding "putting all choice adventures related to the elemental charters in this file, putting all choice adventures related to dreadsylvania in that one").
Doing so allows us to find "missing" choice adventures more easily, as well as duplicates (though there's the test for that one).
I don't think this is a good reason. You can write a test case that shows missing numbers across all files and still organize the files by zone (or whatever is appropriate).

It would also help us deprecate things like 11 year old Cimbo choices, if we wanted to.
 

fredg1

Member
I don't think this is a good reason. You can write a test case that shows missing numbers across all files and still organize the files by zone (or whatever is appropriate).

It would also help us deprecate things like 11 year old Cimbo choices, if we wanted to.
All right, this indeed seems feasible; insistence dropping...
So we're back to: how should they be organized?

Organizing them solely by zone may not be the best option. They are a lot, and I mean a *lot* (but not, like, "half of them" a lot), of choice adventures with an undefined location, as well as some that can appear in more than one location.
 

MCroft

Developer
Staff member
You could put each one in a class file by itself, I suppose.

What's the goal? Make it easy for someone to know where to add or edit? How would you look to find a particular choice?
 

fredg1

Member
Not sure whether "What's the goal" refers to splitting them along multiple files, or the "by choice adventure" refactor altogether, so...

Reasons to split them:

1- Very big files cause editors to do things like mess up the formatting whenever a change is added.

2- In pull requests, GitHub can remember which file you reviewed. Of course, if you have one enormous file, with a few changes sprinkled through it and change *one* thing in it, GitHub will simply say that "the file change since you last reviewed it", but won't say where, so you have to look at the whole file/change previews again.

3- Some choice adventures share logic. As such, this logic goes in static methods. The use of separating choice adventures means that such static methods could always go to the end of the file (easy to find), while staying relatively close from where they are used (and not pile up together).

Reasons for the refactor:
1- Making them easy to reach.

2- Still makes it relatively easy to see what other choice adventures use each method for, but makes it easier to follow how any given choice adventure splits its handling across the various methods.

3- Logic that is re-used in a choice adventure could go as a static method in that choice adventure's class, rather than going in a static method at the end of the file.

How would you look to find a particular choice?
Each choice adventure's definition contains (/will contain) their choice adventure number, as well as their name (I also plan to introduce adding their location to it, but we're not there yet).
 
Last edited:

MCroft

Developer
Staff member
1- Very big files cause editors to do things like mess up the formatting whenever a change is added.

2- In pull requests, GitHub can remember which file you reviewed. Of course, if you have one enormous file, with a few changes sprinkled through it and change *one* thing in it, GitHub will simply say that "the file change since you last reviewed it", but won't say where, so you have to look at the whole file/change previews again.

3- Some choice adventures share logic. As such, this logic goes in static methods. The use of separating choice adventures means that such static methods could always go to the end of the file (easy to find), while staying relatively close from where they are used.
Right. That's the rationale, which is good to know.

But we need a way to break it apart that helps someone who wants to find a choice to edit or test.

Let's come up with a heuristic that breaks it apart. One class per choice, builder pattern, nice abstract choice class to extend, and a file system tree for them that.

That works obviously for zoned choices, so maybe that's the first division. Or maybe whatever we do in the GUI should be reflected in the code.

I'm suggesting that you might have a better feel for this because you've spent more time working with it than I have.

And if it's a file-per-class, then it's pretty easy to rearrange.
 
Last edited:

fredg1

Member
One class per choice, nice abstract choice class to extend
If we had 1 choice adventure per file/class, how should each file/class be named? By its choice adventure name? By its choice adventure number?

Each class still needs to be instantiated. This means that if we have a whole class dedicated to a choice adventure, it still requires a static scope of code making a new instance of it. This is hard to enforce, and easy to overlook...

One choice adventure per class means that we would need one file importing every single one of them? (because, unless I'm mistaken, even static scopes don't get run until its class gets imported?)
builder pattern
I'm... not sure I get what that's referencing.
and a file system tree for them that.
I was hoping to have everything in the same folder, so that we could use the *package* visibility, rather than *public*...
 

heeheehee

Developer
Staff member
Choice adventure names are non-unique (see: Violet Fog for an egregious example, or 342 and 611 for a name that was reused for completely unrelated content).

Static scopes are not run until the class is loaded. Just importing the class is insufficient if you don't use it. There may be value in using reflection with a ClassLoader to manually load these classes.

What are you trying to restrict to package vs public scope?
 

MCroft

Developer
Staff member
By a builder pattern, I’m thinking something like ChoiceBuilder.registerChoice( new Choice1234() ); in a loop or that iterates over a set of classes, possibly from a list, but also possibly from a path, and then builds a data structure for a choice handler.

It might use reflection to get the constructor. Or if it's common enough to have them all be derived from the same abstract choice class, then you may already know what the constructor is called.

The idea is that ideally, you just extend the abstract class and if it's in the right directory it self-registers and then you've got a something like an array in the choice handler keyed by choice # with the class as the argument and and overridden execute function as the action you take.

Am I making sense? The model I have in mind is based on a Discord Bot I develop for; it's registering bot commands, which are written for each bot, with the common command handler. https://github.com/Chew/JDA-Chewtils/tree/master/command
 

fredg1

Member
The idea is that ideally, you just extend the abstract class and if it's in the right directory it self-registers and then you've got a something like an array in the choice handler keyed by choice # with the class as the argument and and overridden execute function as the action you take.
Can't the super/abstract class's constructor(s) simply do the registration themselves?

Like...
By a builder pattern, I’m thinking something like ChoiceBuilder.registerChoice( new Choice1234() ); in a loop or that iterates over a set of classes, possibly from a list, but also possibly from a path, and then builds a data structure for a choice handler.
could just become
Java:
abstract class SuperClass {
  SuperClass() {
    ChoiceBuilder.registerChoice( this );
  }
}

class SubClass extends SuperClass {
}
 

MCroft

Developer
Staff member
Can't the super/abstract class's constructor(s) simply do the registration themselves?

Like...

could just become
Java:
abstract class SuperClass {
  SuperClass() {
    ChoiceBuilder.registerChoice( this );
  }
}

class SubClass extends SuperClass {
}
Looks like it would work. How would you instantiate it?
 

fronobulax

Developer
Staff member
Useless and distracting comments...

ChoiceManager is not on my list of Big Files I never want to see again, RuntimeLibrary is. If we imagine how RuntimeLibrary might be broken up - one class per routine or one file per set or related routines or something eles, that might establish a principal that could be applied to ChoiceManager if the refactoring and reimplementation doesn't.

My approach would be to embed a SQL database and query it for anything related to a choice.

A more Java friendly version would be to have a Choice class that could handle every choice. Using a factory and inheritance the code dealing with choices would be simplified because it would always have the same kind of object. All the fun would be in the factory, but much of that could be table driven. It also makes testing easier since a Choice object can be tested by itself or with arbitrary data and specific Choices can be instantiated for testing.

I think I am endorsing a direction that is already being explored. Think of a parametrized Choice object and separate the concerns of building the object and using it.
 

fredg1

Member
ChoiceManager is not on my list of Big Files I never want to see again, RuntimeLibrary is.
But can you really compare the two? RuntimeLibrary is a collection of arbitrarily defined functions. This means that they can be arbitrarily broken up.
They don't have an "order" other than alphabetically, possibly.
Their existence isn't bound to anything, so we don't have to worry about one being "missing".

ChoiceManager's choice adventures, on the other hand, is a reflection of KoL's choice adventures.
They have a numerical order and a pre-existing name, location and purpose, meaning they could be "incorrectly" grouped with another.
They are bound to KoL, meaning there can actually be "missing" ones or "incorrect" information.

My approach would be to embed a SQL database and query it for anything related to a choice.
I have absolutely no experience with SQL, so I could be talking out of my ass here, but if I get this correctly...

The issue with this is how a lot of the handling involves code, because too much of it is choice-adventure-specific that it can't be organized in any other way, so putting it in a database just seems like an extra, useless step.

A more Java friendly version would be to have a Choice class that could handle every choice. Using a factory and inheritance the code dealing with choices would be simplified because it would always have the same kind of object. All the fun would be in the factory, but much of that could be table driven. It also makes testing easier since a Choice object can be tested by itself or with arbitrary data and specific Choices can be instantiated for testing.

I think I am endorsing a direction that is already being explored. Think of a parametrized Choice object and separate the concerns of building the object and using it.
Isn't it the direction I already took, though?? (though I think this is what you're also pointing out? Not 100% sure; am I correct?)
 

fredg1

Member
I guess a question I need to ask is: have you actually taken a look at the patch, or are you solely basing that comment off of the recent exchanges?

(this isn't a "look before you talk" kind of critique. This is a "I would like to know what information you had access to, in order to rule out some possibilities/scenarios that are currently lingering in my mind", regarding my understanding of your comment)
 

fronobulax

Developer
Staff member
I have previously questioned the necessity for this patch at this time.

I have not looked at the specific patch because it wasn't clear anyone else was discussing the patch. I thought I was in part 1 of a two part discussion. 1) How should we clean up/improve choice handling in general and ChoiceManager in particular? 2) Given an answer to 1) how close is this patch to the answer to 1? Is something else is going on I apologize for butting in, unprepared.
 

fredg1

Member
I have previously questioned the necessity for this patch at this time.

I have not looked at the specific patch because it wasn't clear anyone else was discussing the patch. I thought I was in part 1 of a two part discussion. 1) How should we clean up/improve choice handling in general and ChoiceManager in particular? 2) Given an answer to 1) how close is this patch to the answer to 1? Is something else is going on I apologize for butting in, unprepared.
For 1), Hee^3 mentioned remembering that the unwieldiness of ChoiceManager is a shared feeling, though no citation/quote was added (not that it would've been easy, given that it must have been from hearing it here and there over time...)
My second reaction is that I believe there's general consensus that ChoiceManager is intractable to work with, and arranging things by adventures instead of by when the handling happens is most likely a good thing.
The re-arrangement he mentions is the one happening in the proposed refactor/rework.

For 2), however, the next point mentioned is about how the proposed refactor/rework splits the choice adventures in groups of 100 per file, which felt arbitrary (a single reference was given as explanation).
My third reaction is that arranging this code in classes like "abstract class CADatabase1000to1099" feels wrong, as it requires knowing all the relevant choice numbers (or being able to find it via grep) in order to know how to fix some logic. It introduces awkwardness where if choice adventure numbers for a piece of content cover xx99 - xy00, then I have to look in two separate files. And, there's no enforcement for these files being limited to 100 choices. It's ultimately lumping in a bunch of unrelated functionality (which admittedly isn't worse than the status quo, but it's not really better, either).
And, there's no enforcement for these files being limited to 100 choices.
This could be... an interesting challenge.
Would it help if I were to manage to enforce this?
No. It's an arbitrary convention. I was pointing out the lack of enforcement as yet another reason why I don't like that approach, not the primary reason by any means.

There is currently no objection to the assertion that all choice adventures should *not* just be lumped in a single file, so the current discussion is about how they should be organized.
So... should it really just be one giant file..?
Not necessarily, but arbitrarily splitting it in this fashion isn't a clear improvement.
1- Very big files cause editors to do things like mess up the formatting whenever a change is added.

2- In pull requests, GitHub can remember which file you reviewed. Of course, if you have one enormous file, with a few changes sprinkled through it and change *one* thing in it, GitHub will simply say that "the file change since you last reviewed it", but won't say where, so you have to look at the whole file/change previews again.

3- Some choice adventures share logic. As such, this logic goes in static methods. The use of separating choice adventures means that such static methods could always go to the end of the file (easy to find), while staying relatively close from where they are used.
Right. That's the rationale, which is good to know.

But we need a way to break it apart that helps someone who wants to find a choice to edit or test.


  • Should there be a tree of folders, organizing choice adventures in groups, then sub-groups? What should the organization be based on?
  • Should there only be a single choice adventure by file? Considering that said file can't be named by that choice adventure's name, as some choice adventures sometimes share names (even a rare few that are completely unrelated). There's also the fact that every single file still needs to be loaded from something else.
 
Last edited:

MCroft

Developer
Staff member
I have absolutely no experience with SQL, so I could be talking out of my ass here, but if I get this correctly...
Think of a SQL DB in this case as a better way of storing and joining data that we currently keep in flat files, with better tools for relating and searching, including adding and deleting things. If we put a Postgres/MariaDB/SQLLite3 DB in the product, we'd centralize a lot of our data handling with the possibility of causing new issues. We at least wouldn't have tab vs. space issues.

The issue with this is how a lot of the handling involves code, because too much of it is choice-adventure-specific that it can't be organized in any other way, so putting it in a database just seems like an extra, useless step.
You can put code into a DB, and a DB can run code inside the DB, but this is the crux of a lot of the discussion over the last few months, isn't it? If too much of it is choice-adventure-specific that it can't be organized in any other way, then maybe it's not worth changing. It's hard to drum up excitement for a re-write when your collaborators don't see it as being a radical enough change to justify the cost/risk. I'm not sure anyone else has been convinced that can't is necessarily true, but we could be wrong.

We have a clunky solution that has grown over years into a very large clunky solution. It could use a re-write, although as Frono points out it isn't sitting at the top of anyone's ToDo list. But that's how open-source works; what gets worked on is what is interesting to the contributors, so that's not the barrier.

If you (or collectively, we) come up with an elegant solution, it could be easy to convince the dev team to make a change. If it looks like it will just leave the same problem for later, it's a harder sell. And between two equal solutions, the tiebreaker is going to go to the fielded code, since it has so many more hours of runtime testing. Better the devil we know.

tl;dr-- please consider what the threshold for accepting the changes are, and address suggested improvements as ways of helping you overcome them.
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
I think with this and with many other places in the codebase we could look at using something like the Reflections library to "discover" all the classes in a package, and run ChoiceBuilder.registerChoice(new JustDiscoveredChoice()) like that

 

Veracity

Developer
Staff member
I think having a ChoiceAdventure class with one instantiation per choice and data and methods to replace giant switch statements and the data structure used to control which choice adventures and options are visible in the GUI seems good.

The visitChoice, preChoice, postChoice0,1,2 methods seem necessary; there are very specific places in GenericRequest where each of those is called, and every time we've done major munging (such as when I made multi-fights and multi-choices (and mixes of the two) accumulate all item drops so that only at the very end do you see everything complete with use links), it's been a big job to make sure that everything still works, both in automation and in the Relay Browser.

I have never liked the static globals in FIghtRequest, GenericRequest, and ChoiceManager. It is true that your character can't ever actually be in more than one fight or choice at a time, but that's still tricky and fragile.

Regarding whether you should split the choices into groups, I have no objections to large files. I propose something like this:

ChoiceAdventureData.java
- the class with fields and methods for one choice

ChoiceManager.java
- data structures (hash tables? arrays?) of ChoiceAdventure objects.
- top-level methods called from GenericRequest and other parts of KoLMafia that take a choice#, look up the object, and call methods, retrieve data, whatever, as needed.
- a registerChoice method which takes a ChoiceManager object and is called at the end of the creator method to update all the data structures as needed. Or perhaps to simply add all ChoiceAdventure objects into a TreeSet, and then all the other data structures can be created from that Set after all the choice objects have been initialized. Whatever.

ChoiceAdventures.java
- a single large file which creates every ChoiceAdventureData object as static initialization. Perhaps even just a series of calls like this in a static block:

Code:
      new ChoiceAdventureData (
          param1,
          param2,
          ...,
          paramN ) {
        @Override
        public final void visitChoice(final GenericRequest request ) {
          ...
        }
        @Override
        public final void PREChoice(final GenericRequest request ) {
          ...
        }
        ...
      };
Perhaps there is choices.txt with a lot of constant data that can be read once and used at object creation time, rather than calling a BOA Constructor like this (or like CoinMasterData)
Perhaps the TreeSet of choices is in this file.
Perhaps ChoiceManager, which imports this file, static initializes its own data structures from that Set.
Whatever; those are all design choices.

This is wild brainstorming; this is not a proposed design. But it does accomplish what I think are desirable goals:

- We retain the carefully defined-over-years external interface to ChoiceManager
- Everything that differs between choices is captured in a ChoiceAdventure object
- There still is a large file (meh), but adding a new choice is nothing more than adding a new static object creation in the appropriate (numeric) place with exactly the method overrides needed to make it work.

Why do we care about large files that define data (like this) or methods (like RuntimeLibrary)?
 
Top