[RFC] [1.17.x] Remove cases where null is returned to reduce failure potential.
marchermans opened this issue ยท 3 comments
Reducing the cases where null values are returned.
This was observed during reviews of several PRs the last few weeks.
We should do a scan of cases where we either intentionally or unintentionally return null to indicate some processing state.
This should be changed to return a none null value, potentially with better processing logic included internally.
Most notable places:
- Our state machine accepts null as a returned value.
Well I was actually thinking of going a 4. route:
- Actually return a different type altogether, something like an
Optional<IAIState>or something custom that defines behavior better.
I have spent some time looking at this, and have really mixed feelings about this. I see 3 options:
- Refactor the AI that uses nulls today to be boolean providers, and use explict states to transition to if they return true
- Create a new "NEXT" state, this effectively is a drop in replacement in our code for "null".
- Stick with "null" having special meaning when returned during a transition.
Choice 1 will likely require a few extra AITargetEvents that we don't have today to manage the transitions, and we're doing real, critical, processing during a function that SHOULD be a lightweight "do we need to process". This feels very hacky to me.
Choice 2 doesn't seem to move the needle in my opinion, and actually feels like a step backwards. A new state makes it clear that we're doing something special, but it's a state that has very special handling. We wouldn't save it as the current state, but rather move on to the next one. Our basic state machine actually combines checktransition and transition by chaining the latter as the return of the former.
Choice 3, do nothing seems like the best choice at the moment to me. While it retains the special processing around a specific 'flag' value, it's a value that is clearly not something that we would retain for further use, where "NEXT" or whatever state we use as an alternative has the possibility of retention and thus misuse/bugs.
I would love to hear of a better choice. I know there are some options with major refactors of the state machine handling, but those carry a much higher risk to stability, for what is pretty fundamental in our code, and pretty reliable today.
