Feature ChoiceManager refactor

fredg1

Member
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.

This is the very downside I was preoccupied with regarding the "split big changes into small steps" approaches: the individual step's values are scrutinized without the knowledge of how they help/fit into a broader change.

This rework is just meant to be a start.
The goal is to end up being able to log which choice takes a turn/increments the turn counter in a zone, which zone they occur in, have clearer/dynamic information as the choice adventure spoilers...

The issue is that this information often gets modified by some other context (for the spoilers), and not all choices cost a turn for a some choice adventure.
Storing handling by choice adventure allows one to see how a choice adventure's information gets altered over the various methods, which is why it is the first step.
 

Veracity

Developer
Staff member
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.
I object to that assertion. It seems completely unnecessary to me.
I want to get away from numerous switch statements based on choice number, but multiple objects (or subclasses) in one file seems just fine to me.

Multiple objects - add to a set when objects are created?
Multiple classes - use gausie's Reflections idea.
 

fredg1

Member
Code:
      new ChoiceAdventureData (
          param1,
          param2,
          ...,
          paramN ) {
        @Override
        public final void visitChoice(final GenericRequest request ) {
          ...
        }
        @Override
        public final void PREChoice(final GenericRequest request ) {
          ...
        }
        ...
      };
This is wild brainstorming; this is not a proposed design. But it does accomplish what I think are desirable goals:
This is more than wild brainstorming; this is extremely close, if not a complete match, with what I went with:

Excerpt:
Java:
package net.sourceforge.kolmafia.persistence.choiceadventures;

public class ChoiceAdventureDatabase {
    private static final Map<Integer, ChoiceAdventure> database = new TreeMap<>();
    static final List<Integer> duplicates = new LinkedList<>();

    public static class ChoiceAdventure {
        void postChoice2(GenericRequest request, int decision) {}


        public final int choice;
        public final String name;

        ChoiceAdventure(int choiceAdventureNumber, String name) {
            this.choice = choiceAdventureNumber;
            this.name = name;


            if (database.containsKey(this.choice)) {
                return;
            }

            database.put(this.choice, this);
        }
    }

    public static final class UnknownChoiceAdventure extends ChoiceAdventure {
        UnknownChoiceAdventure(int choiceAdventureNumber) {
            super(choiceAdventureNumber, null);
        }
    }

    /** Bridge between {@link ChoiceManager#postChoice2} and {@link ChoiceAdventure#postChoice2} */
    public static final void postChoice2(int choice, GenericRequest request)
    {
        int decision = ChoiceManager.getLastDecision();
        getChoiceAdventure(choice).postChoice2(request, decision);
    }

    public static final ChoiceAdventure getChoiceAdventure(int id) {
        ChoiceAdventure choiceAdventure = database.get(id);

        if (choiceAdventure == null) {
            return new UnknownChoiceAdventure(id);
        }

        return choiceAdventure;
    }
}

//////////////////////////
        [...]

        new ChoiceAdventure(3, "The Oracle Will See You Now") {
            @Override
            void postChoice2(GenericRequest request, int decision) {
                if (request.responseText.contains("actually a book")) {
                    Preferences.setInteger("lastPlusSignUnlock", KoLCharacter.getAscensions());
                }
            }
        };

        new ChoiceAdventure(7, "How Depressing") {
            @Override
            void postChoice2(GenericRequest request, int decision) {
                if (decision == 1) {
                    EquipmentManager.discardEquipment(ItemPool.SPOOKY_GLOVE);
                }
            }
        };

        [...]
 

fronobulax

Developer
Staff member
Well I can't seem to shut up.

Large files are primarily a problem because I spent my career working in shops where code that took more than two "pages" to display was a possibly indicator of poor design or implementation choices. So my knee-jerk reaction to large files is to ask whether they have to be that large. Any good book on how to write effective Java will at least discuss the file size concerns.

My tortured history aside there are cases where files are too big for our coverage tools and too big to display a diff by default. As a reviewer I do feel I have given a better review if the file under review is of a manageable size.

veracity pretty much said what I was trying to and with more clarity.

My unresolved concern and it is based on the patch, is why do we have things organized by numerical choice values?
 

fredg1

Member
My unresolved concern and it is based on the patch, is why do we have things organized by numerical choice values?
Well, that was kind of what some of the latest discussion was about: how else?

They can't be sorted by name because of conflicts
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).

And if the sorting is done arbitrarily, you'd need to make sure one choice adventure can't fit in more than one "category" at once, otherwise someone will look into one of the categories and assume the choice adventure is missing...
(in other words: it needs to not make it too hard to find a given choice adventure, or lack thereof)
 

heeheehee

Developer
Staff member
Why do we care about large files that define data (like this) or methods (like RuntimeLibrary)?
One argument against giant methods is a technical limitation of Java bytecode: methods cannot exceed 65535 bytes.

This is not purely hypothetical, either: when we were briefly using OpenClover for coverage, we ran into this limitation for ChoiceManager.postChoice1 and UseItemRequest.parseConsumption.

The code looks up choice adventures by ID, so I sorted the choice adventures by ID.
How the logic is partitioned into files doesn't have to mirror the internal data structure that holds them.
 

MCroft

Developer
Staff member
How the logic is partitioned into files doesn't have to mirror the internal data structure that holds them.
How the files are named and what directories they are in should help developers find them. Since it also has a unique name requirement, we could reasonably adopt the naming convention of the wiki: e.g. class violetFogManOnCornFlake() extends Choice vs class violetFogFogOfBirds()
 

MCroft

Developer
Staff member
Well I can't seem to shut up.

Large files are primarily a problem because I spent my career working in shops where code that took more than two "pages" to display was a possibly indicator of poor design or implementation choices. So my knee-jerk reaction to large files is to ask whether they have to be that large. Any good book on how to write effective Java will at least discuss the file size concerns.

My tortured history aside there are cases where files are too big for our coverage tools and too big to display a diff by default. As a reviewer I do feel I have given a better review if the file under review is of a manageable size.

veracity pretty much said what I was trying to and with more clarity.

My unresolved concern and it is based on the patch, is why do we have things organized by numerical choice values?
I like the new style of short files, for all the tooling reasons that have already been mentioned and also for making the codebase more approachable to newer developers. Short files help me understand the code by providing more frequent road signs. If another new developer wants to work on the Choice system, it's easier for them to approach it and us to review it. If they get into trouble, it will be in a smaller unit of work. Unit tests are easier to scope.

All that said, it's a preference that may not make sense for someone with 15 years in the codebase.
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
I basically completely agree with @Veracity's proposed approach. I do (on balance) prefer a smaller file, but I think we can have the same architecture in a way that feels more "modular" by using the reflections library I posted before.
 

Veracity

Developer
Staff member
One argument against giant methods is a technical limitation of Java bytecode: methods cannot exceed 65535 bytes.
I agree. I do not like giant methods. That is why I hate the current postChoice1 method, for example, which has a gigantic switch statement indexed by choice number.

So, making postChoice1 do a hash lookup (say) of a Choice object which has a postChoice 1 method inside it to do exactly what is special for that choice number and replacing exactly the switch statement with a method call on the object does a fine job of reducing all the methods to a page or two.

I proposed that we have a file with nothing but static initialization of said choice objects:

Code:
static {
    new ChoiceAdventureData( 1, parameters...) { ...method overrides... };
    new ChoiceAdventureData( 2, parameters...) { ...method overrides... };
};

If I understand correctly, gausie is proposing subclasses:

Code:
    public class ChoiceAdventure1 extends ChoiceAdventureData { ...method overrides...}
    public class ChoiceAdventure2 extends ChoiceAdventureData { ...method overrides...}
(Are you required to provide a Constructor for subclasses?)

I don't understand how "maximum byte code in a method" affects either of those techniques.
Unless a "static" block at top level is actually a method, somehow?
 

heeheehee

Developer
Staff member
I don't understand how "maximum byte code in a method" affects either of those techniques.
Unless a "static" block at top level is actually a method, somehow?
It does not. But it is also subject to a similar limitation.

I copy-pasted 17k lines in a static block and triggered "Code too large".

I believe it's this part of the classfile spec I ran into.

(Are you required to provide a Constructor for subclasses?)
(You aren't required to provide a constructor in Java, period.)
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
My issues with it haven't been addressed and so until either you do so or I find the time to do so, there have been no updates.

Have you changed your approach at all based on the discussion of the 24th/25th of October?
 

fredg1

Member
My issues with it haven't been addressed and so until either you do so or I find the time to do so, there have been no updates.
Well, it seems like those issues have not been clearly stated.

Have you changed your approach at all based on the discussion of the 24th/25th of October?
There have been two "issues" raised so far: the (possible) separation of the choice adventures into more than one file, and the method used to "make" each choice adventure.

In that discussion, no conclusion was reached, and the discussion simply suddenly stopped.

The first issue was met with the argument that it shouldn't be split at all. Reasoning has been added/brought up as to why it would eventually be necessary, and then nothing.

For the second issue, two relatively similar methods have been brought up.
You also briefly mentioned a "reflection library", but seemed to assume everyone would just know what it would imply and didn't actually extend on that (e.g. examples). Veracity showed what her understanding of that proposal was, and I fail to see how it is substantially different from either of the two previously proposed methods.

I fail to see how any of this would result in me "changing my approach", especially how said approach was not mentioned at all in this discussion.
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
So just to clarfiy: this thread had some discussion on what needed to change in order for your work to be merged and you've made exactly no changes and bumped it asking if there have been "any developments"?
 

heeheehee

Developer
Staff member
In that discussion, no conclusion was reached, and the discussion simply suddenly stopped.
If you're the most invested in getting these changes merged in, then you have to recognize that you need to drive the discussion forward when it gets stalled. And no, "bump" isn't driving the discussion forward. Summarizing your understanding of the discussion is a good start, but also asking more pointed questions like "I think my proposal satisfies the raised concerns because X, Y, and Z. @gausie can you elaborate on any other concerns I missed?"

(Aside: I don't know if it's common knowledge, but we staff members can see edit history and deleted posts. While having the hindsight to delete posts made in anger is worth something, said posts can still be held against you.)

Attacking any of the individuals who can accept your changes is likely to alienate the rest of us and discourage us from interacting directly with you in the future.

You also briefly mentioned a "reflection library", but seemed to assume everyone would just know what it would imply and didn't actually extend on that (e.g. examples). Veracity showed what her understanding of that proposal was, and I fail to see how it is substantially different from either of the two previously proposed methods.
@gausie also provided a link to said reflections library. But okay, since it's apparently not clear, the idea is that instead of having one giant "register all the choice adventures" method that hardcodes them and needs to be updated every time you add a new choice adventure, you'd have a method that looks for all the classes in a specially-named package, and registers them programmatically.
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
(Aside: I don't know if it's common knowledge, but we staff members can see edit history and deleted posts. While having the hindsight to delete posts made in anger is worth something, said posts can still be held against you.)
Some of those are me moderating aggressive, vexatious or unhelpful posts.
 

fredg1

Member
Summarizing your understanding of the discussion is a good start, but also asking more pointed questions like "I think my proposal satisfies the raised concerns because X, Y, and Z. @gausie can you elaborate on any other concerns I missed?"
But there was no proposal yet.
The
this thread had some discussion on what needed to change in order for your work to be merged
statement was, objectively, false.
 

heeheehee

Developer
Staff member
there was no proposal yet.
Then what are we supposed to work off of?

When I say "my proposal" in the above quote, I'm specifically imagining myself in your situation. That is, "the patchset that I, fredg1, provided in this thread, is immune to all of your concerns, and here's why" [0]. Or, "I, fredg1, propose that we take this specific approach that satisfies these concerns raised by Veracity, gausie, and heeheehee. I recognize MCroft's concerns, but there's frankly no way to reconcile X with Y raised by gausie."

[0] I at least think it's obvious you wouldn't want to word it that way, lest you come across as arrogant.

The statement was, objectively, false.
Are you saying that none of the discussion in the thread was at all relevant to your proposed patch?

Like, I get the "too many cooks in the kitchen" situation we had going on once we had 5 or so distinct devs chime in with their individual opinions, but none of it, really?
 
Top