Static final

fronobulax

Developer
Staff member
I have previously admitted my incomplete knowledge of the subtleties of static and final keywords in Java so this is a probable but unconfirmed hypothesis and a cautionary tale for folks who manipulate Java.

I often run IntelliJ's inspect function on code I am going to commit anyway in the belief that "inspected" code is more robust or easier to maintain. Often IntelliJ will suggest a static field could be declared final and I accept the suggestion.

I was writing tests for EquipmentDatabase. Everything worked. I made changes I wanted to EquipmentDatabase and declared some fields static final as suggested. Tests failed, but not mine. The failure indicated a problem with static initialization (and was in a test that used EquipmentDatabase).

EquipmentDatabase has a static block that calls a function that reads a file and populates fields.

I believe the static analysis saw that the function was the only thing that set some fields and thus made the suggestion. But there are circumstances where the function is called elsewhere or the EquipmentDatabase object is recreated and making the fields final prevents reinitialization.

I will address this in the short term by not believing static field can be declared final without further investigation but I mention it as a warning in case other folks also like to remove lint.

Long term, there are probably some implementation details that should be reviewed. I can often argue in favor of reducing the number of static declarations. We have probably violated some kind of recommendation by initializing what I changed to static final fields in a publicly accessible method. We may want better separation of fields that we know are really final and ones that can be reset by an extraordinary event, such as changing character or breaking the prism.

At the moment this is a cautionary tale that may be seriously revised as I actually make and undo changes to get things to work.
 

fronobulax

Developer
Staff member
And in a classic example of overthinking merely changing

private static final IntegerArray power = new IntegerArray();

to

private static final ArrayList<Integer> power = new ArrayList<>();

breaks tests that do not directly involve EquipmentDatabase.

Fun, fun, fun, but I will pursue.
 

fronobulax

Developer
Staff member
Someone who doesn't like to admit mistakes in public and has Moderator access might consider deleting this thread. But not me.

I am trying to replace our homebrew IntegerArray with native Java structures now that we have adopted a version of Java that has them.

However, our version will populate the array with zeros if asked to set an index that is beyond the current size of the array. As described this could be wrong behavior but I suspect my description is what is wrong. My choice for a native data structure doesn't work that way and thus things that used to be there are either not there or returning an "incorrect" value. I need to reconsider my choice of native structure and explore the possibility that the homebrew IntegerArray cannot be trivially replaced.
 

heeheehee

Developer
Staff member
Regarding static final: these qualifiers, when attached to a field, mean that the object is 1) not bound to an instance of the class, but rather to the class itself, and 2) cannot be reassigned.

Static blocks are (logically) something else entirely, and refer to static initialization, which is done the first time Java loads the class in question. That's probably what's breaking other tests (MaximizerTest in particular, I'd imagine).

Would it make sense to instead use a Map<Integer, T> of some flavor? This "insert zeroes if setting out-of-bounds elements" behavior assumes that the values are dense in order for this to actually save space; otherwise, a HashMap lookup should be nearly as fast (expected constant time, cheap (trivial) hash for Integer, extra modulo operation; cache coherence should be comparable) and potentially more space-efficient.

You'd replace sets with puts, but really, with something like EquipmentDatabase, we should be organizing the data better than "five specialized arrays" (e.g. use an inner class with fields for power / hands / itemType / statRequirements / pulverize).
 

fronobulax

Developer
Staff member
Well a straightforward replacement of our array with some flavor of Map doesn't work. We have code that relies on the array being fully populated and the refactoring to bypass that seems to be something that should wait until we have moved to GitHub and a code review or discussion can be more easily facilitated. So I will redirect my energy back to tests that increase coverage and/or tests that can be a before and after check when the refactoring and deprecation of our classes occurs.
 

fronobulax

Developer
Staff member
The Maximizer test triggers an update/adjustment of all modifiers. The debugger tells me that addItemAdjustment in KoLCharacter gets called with an AdventureResult that yields an item with id -1 and name "(none)". addItemAdjustment will bail for a null item or something being unequipped but it gladly tries to get the power of item ID -1. Our code returns zero if the array is accessed with an index out of range. The Java data structures do not. Hence the Maximizer test failure when I change the underlying storage in EquipmentManager.

I'm inclined to handle the problem in KoLCharacter but probably won't get a chance to try today.
 

heeheehee

Developer
Staff member
That item of -1 is very likely EquipmentRequest.UNEQUIP.

With something like a Map, you'd want getOrDefault(EquipmentRequest.UNEQUIP).
 

fronobulax

Developer
Staff member
Is there a process for an idea review before it becomes code?

The current code is just going to return a lot of zeros that are not actually in the data but are the result of accessing an out of bounds index. If those zeros actually have any meaning then it bothers my sense of elegance and good design that the desired behavior is the result of how something is stored.

Maybe the solution is to remove the suggestion that IntegerArray (et. al.) can be replaced with native classes and document the quirks in comments?

Maybe a Map is used and initialization includes adding an entry for item -1 ? Then the change in behavior only occurs with item numbers that are greater than what is defined in equipment.txt.


Maybe I got me a new test to write...
 

heeheehee

Developer
Staff member
You normally should be able to file a feature request in Issues, but we've apparently disabled that feature in favor of issue tracking on this forum.

edit: there's also a "discussions" feature that's turned off for our repo. I enabled it for my fork.
 

fronobulax

Developer
Staff member
You normally should be able to file a feature request in Issues, but we've apparently disabled that feature in favor of issue tracking on this forum.

edit: there's also a "discussions" feature that's turned off for our repo. I enabled it for my fork.

I'm fine with doing idea reviews here and in a less structured fashion. Maybe we just socialize the idea that the Feature tag should be used for such discussions? I tend not to think of it that way although I can't figure out why not.
 

heeheehee

Developer
Staff member
I think that makes sense, or a post in Scripting Discussion (although last time I did that, nobody posted on it, and then I decided to commit my patch anyways).
 
Top