Feature ChoiceManager refactor

[fredg1]

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?

Don't mix "relevance" with "it was the subject".
It is "relevant" to my proposed patch (which was not proposed in this thread), yes, but it was not "mentioned" here.
The statement was that the recent discussion was about what needed to changed in said proposal for it to be accepted. That much is false.

(There was "a patch" uploaded in this thread, but it's use was only to show the proposed organization)

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"

But that's the issue: I did not assert that it was satisfying everyone's concern. That's why I was asking veracity if she still held hers after the previously mentioned arguments.
It's why I haven't made a proper proposition yet. Because we were still on the topic of "how should choice adventures be organized", and the result of that topic would heavily alter said proposal.

 

[fronobulax]

Because we were still on the topic of "how should choice adventures be organized", and the result of that topic would heavily alter said proposal.

"Bump" does not send me that message. Repeating the question "how should choice adventures be organized" does.

 

[Ryo_Sangnoir]

I don't know what gausie's concern is; I don't see it in this thread. Can we have it in this thread, please?

To chime in with /another/ opinion: I personally think that one big file is preferable to ~14 and increasing smaller files each of which have ~100 choices, and also preferable to ~1400 small files named "ChoiceAdventure1234" and the like. I think rearranging the methods inside smaller classes is fine, but I don't think the smaller classes should then be moved out (unless they have some weight to them).

I am somewhat opposed to gausie's reflection library solution unless it works like Dagger and generates some code at compile time. Moving things that currently would fail at compile time to things that would only fail at runtime seems wrong to me. Additionally, from experience working on a project that did the same thing, there's a small performance penalty (increasing as the number of reflected classes increases).

I would support Veracity's approach.

 

[fronobulax]

I'm not going to reread and see whether I, or anyone else, has said this before.

Having worked in an environment where well defined objects were important I am inclined to favor smaller files and more of them. I also understand that with several hundred choices I would soon regret that decision.

If I had the time, energy and resources to impose a solution and have my team build it or tell me why I was wrong, I would start with some kind of Choice object and a factory that built one based upon a parameter passed to the factory. The factory would be backed by some kind of datastore. I can imaging an initial implementation where the easy choices are constructed by data and the complex choices by a wrapper around existing choice management code.

I am personally wary of reflection. Some of that is just experience since similar functionality can be provided by defining and implementing an Interface. The only objective concern I have is that reflection can sometimes break static analysis of code but since in KoLmafia much of RuntimeLibrary is not called by code the utility of static usage analysis is already limited.

 

[Veracity]

I hate the idea of arbitrarily splitting up the cases into multiple files. If they are split by choice number, then I need to lookup the choice number, rather than searching by encounter name, say.

I am not in favor of reflection for this.

 

[Veracity]

I'm considering taking a first step for this by moving all the choice-specific stuff out of ChoiceManager into another file.

ChoiceManager will have:

- The small number of static variables that I don't really like, but considering that a choice can be active across multiple RelayRequests arriving from the browser, they can't really be instance variables. Perhaps we can eventually get it down to one - "the choice that is currently active". Which is probably how KoL does it.
- Support for automating choice chains via CHOICE_HANDLER - which is a single GenericRequest.
- Methods that are called by GenericRequest to do choice-specific things
- Ditto for RequestEditorKit for choice spoilers

All the rest will go into a new file.

I want to study, in particular, the methods/variables we use across multiple requests, and it's a bit unwieldy right now.