PObject - The Refactoring

Introduction

NOTE: This discussion has been resolved by using a reference-based system (a relative of solution #3 below). See the CDOM References Concept Document for more information

PObject

PObject is a massive object that contains some good stuff:

• Default implementations
• Default information (its member variables)
• Implementations for functionality that is considered 'Global'

And some not so good stuff:

• Specific implementations (e.g. Specific adding Cross Class Skill methods)

Abstract out PObject

It's not easy to abstract PObject out as we determine what sort of PObject to create based on the file type specified in the PCC. So for example when we are loading a EQUIPMENT:equipment.lst file we create Equipment objects. The problem I was highlighting is that we don't have enough information to create a MagicEquipment object until we read the line from the equipment file and see a TYPE:Magic indicator. By that time we already have an Equipment object created.

Solution Proposal 1

Casting an extra time to a Child Object

• Aaron Divinsky: One approach could be to create the base Equipment object but when a more specific object is encountered (via a TYPE tag for example) create the more specific item and copy the values from the base object and delete it. This would require a fair amount of rework of the loading code.

• Devon Jones: Probably not appropriate for the Magic Items as equipment example, but a class that is a parent class for Spells, Psionics, etc would be good. As we deal with more systems, I'm sure we'll see more and more conceptual ideas that are loosely based on abilities that you can use a limited amount of times in a certain amount of time (be it spell/psionic points, or just casting a certain number of spells per day per level). There is no way we could make this kind of abstraction in equipment, and that's fine. This was more intended to seperate the implementations of various magic systems that are functionally different (the largest split I know of is psionics vs normal D&D spells, but the warlock class with it's unlimited casting of chosen abilities may actually represent something that should be backed with it's own object type (or maybe not)).

Solution Proposal 2

Factory system

• Brad Stiles: What about some sort of factory system, which parses the text of the entry, then emits the appropriate object? Not that I'm overly fond of factories, but sometimes they do have their place, and it would seem to be one solution.

Solution Proposal 3

Two Pass approach

• Aaron Divinsky: The other approach I have thought of is to have a two pass approach for object creation. The first pass reads the files and creates the objects using whatever information we need to do that (e.g. TYPE tags). Then we do a second pass as we do now but pass the objects we created in the first pass. This has the advantage that all objects that will be created are referencable at load time by all other objects. This means that you don't have to store as many things as strings and can resolve them at load time instead.

Solution Proposal 4

Helper objects

• Brad Stiles: What about attaching helper objects to the base object that modify it's behaviour through a defined interface? If objects can have more than one type, this one would allow all those types to coexist with each other so that the base object doesn't need to know that it's a "magic" or "adamantine" type, just that it needs to query the list of helpers to find out how to modify it's behaviour.

Solution Proposal 5

Interfaces

Tom Parker: First let me note that I think we are referring to two different problems here. My solution here is not attempting in any way to solve the base problem stated above (that the type of PObject is based on the file being imported and we can't make it more specific). I think that is a non-issue today, and there are good solutions to that problem above (3 and 4 are both good). The problem to which I am referring is that the use of abstract classes rather than interfaces come with baggage in the form of problematic code and code cycles (or tangles, if you prefer). I firmly believe this is a code problem in PCGen that causes changes in one area of the code to chain long distances to other places in the code as a result of interdependent code. One of the fundamental purposes of an Interface is to limit the scope of visibility between objects, such that what may be a complex object is behind a simple Interface. As long as the Interface is maintained, the complex Object can be split, rewritten, mangled, and otherwise completely changed without impacting the code which uses the Interface.

The reason I refer to Language, et al. as potential Interfaces is not because they won't contain information (of course there has to be a concrete object underlying the Interface), but because they are almost universally constructed in one place and used in another (and generally used as read-only, although there are exceptions to that)

Looking at Language (as a very simple example), there are a few things that stand out to me:

1. Use of PObject as a parent class of Language produces (in many cases) a very heavy weight object. What could be 100 bytes in memory is actually >> 1000 bytes. In theory, one might want to have a LightWeightLanguage for Sylvan and other simple languages and a FullWeightLanguage for Druidic and others with special considerations (such as prerequisites).

Sylvan TYPE:Spoken.Written.Read.Druid Druidic TYPE:Spoken.Written.Read.Druid.Secret PREVARGTEQ:DruidicLanguage,1

Having said that, there would still likely be an abstract PObject-like class for storing the heavy-weight common elements from which FullWeightLanguage would be derived... so there wouldn't be duplication of fields/code.

While the general reaction to factories has been poor (and I agree they are a pain, in general), I'm not sure this means factories in any way. One could easily have a FullWeightLanguage that took a LightWeightLanguage in the constructor (for initialization) and the "factory" would really just be code in the LST import code (LanguageLoader.parseLine), because:

2. Languages (and most other PObjects) are only constructed in three places: (A) pcgen.persistence.* (during LST import), (B) pcgen.gui.* (The LST editor), and (C) test code [doesn't really count]. Therefore, while PCGen is actually processing a character, there is no reason to be able to construct or generally modify a Language object. Therefore, the Language interface should be read-only. While this is an abstraction of sorts, it would likely provide a much simpler set of methods to be called by a user of one of the PObject types, since there would be no assumptions about what the underlying Class was. This simplifies the list of stuff the user of a Language needs to sort through to understand what is happening and allows the import folks to modify the underlying object as long as the interface isn't changed. Fundamentally, this reduces circular code dependencies (tangles, whatever terminology you choose), which is a huge problem in PCGen. (These dependencies are part of what causes changes to impact so many pieces all at once).

As a side note, such a read-only Interface for Language and the other PObjects would also prevent any future situations like PCClass. If the user of an Object only knows the Interface of that Object (and that is read-only) then there is no chance of modifying the Object. This type of abstraction forces folks to think about what they are doing and the purpose of the Object they are working with.

3. PObject is a catch-all. When you see a method can handle a PObject, it's really not helpful, because that's a series of a bazillion methods. It would be much more helpful to have code that processes KeyedListContainer or SourcedObject... my thinking is that the eventual "PObject" would simply be a super-interface of 5 or so other interfaces (hence my comment about unwinding). Doing that forces better code containment (again, less impact across code) and reusability (for what they may be worth), since the code is generally written to simpler interfaces.

...and having looked at PCClass:

4. Interfaces are useful for abstraction. The challenge with PCClass is actually a pretty good risk of a bug going forward. It seems the best solution for PCClassLevel will be to have a PCClass embedded within PCClassLevel (has-a relationship) and used for delegation of specific items. However, because PCClassLevel will likely have to be a PObject as well, this introduces risk. When a new method is added to PObject, if the creator of that method does not correctly delegate from PCClassLevel into PCClass, then the proper information will not make it into the PlayerCharacter (because the default implementation of the class in PObject will not have the information that was loaded into the PCClass but what was loaded into the PCClassLevel (which is nothing). With an Interface, the Interface would be implemented by PCClassLevel and thus, the development environment or compiler would flag PCClassLevel as not having the new method. Thus, with abstract objects being extended, we are forced to have testing catch errors, whereas the compiler would do it for us with Interfaces.