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.)
 
Top