[Refactor] getPokemonById returns undefined instead of null

https://github.com/pagefaultgames/pokerogue/pull/6544

* [Refactor] Make `BattleScene.getPokemonById` return `undefined` instead of `null`

* fixed substitute unit test :(

* Update src/phases/obtain-status-effect-phase.ts

Co-authored-by: Sirz Benjie <142067137+SirzBenjie@users.noreply.github.com>

* Update src/phases/pokemon-phase.ts

Co-authored-by: Sirz Benjie <142067137+SirzBenjie@users.noreply.github.com>

* Update battle-scene.ts comment for dean

* Add todo comment

---------

Co-authored-by: Sirz Benjie <142067137+SirzBenjie@users.noreply.github.com>
Co-authored-by: NightKev <34855794+DayKev@users.noreply.github.com>

src/battle-scene.ts

@@ -858,20 +858,21 @@ export class BattleScene extends SceneBase {
   }
 
   /**
-   * Return the {@linkcode Pokemon} associated with a given ID.
-   * @param pokemonId - The ID whose Pokemon will be retrieved.
-   * @returns The {@linkcode Pokemon} associated with the given id.
-   * Returns `null` if the ID is `undefined` or not present in either party.
-   * @todo Change the `null` to `undefined` and update callers' signatures -
-   * this is weird and causes a lot of random jank
+   * Return the {@linkcode Pokemon} associated with the given ID.
+   * @param pokemonId - The PID whose Pokemon will be retrieved
+   * @returns The `Pokemon` associated with the given ID,
+   * or `undefined` if none is found in either team's party.
+   * @see {@linkcode Pokemon.id}
+   * @todo `pokemonId` should not allow `undefined`
    */
-  getPokemonById(pokemonId: number | undefined): Pokemon | null {
+  public getPokemonById(pokemonId: number | undefined): Pokemon | undefined {
     if (pokemonId == null) {
-      return null;
+      // biome-ignore lint/nursery/noUselessUndefined: More explicit
+      return undefined;
     }
 
     const party = (this.getPlayerParty() as Pokemon[]).concat(this.getEnemyParty());
-    return party.find(p => p.id === pokemonId) ?? null;
+    return party.find(p => p.id === pokemonId);
   }
 
   addPlayerPokemon(

src/data/arena-tag.ts

@@ -138,7 +138,7 @@ export abstract class ArenaTag implements BaseArenaTag {
     }
   }
 
-  onOverlap(_arena: Arena, _source: Pokemon | null): void {}
+  onOverlap(_arena: Arena, _source: Pokemon | undefined): void {}
 
   /**
    * Trigger this {@linkcode ArenaTag}'s effect, reducing its duration as applicable.
@@ -172,9 +172,8 @@ export abstract class ArenaTag implements BaseArenaTag {
   /**
    * Helper function that retrieves the source Pokemon
    * @returns - The source {@linkcode Pokemon} for this tag.
-   * Returns `null` if `this.sourceId` is `undefined`
    */
-  public getSourcePokemon(): Pokemon | null {
+  public getSourcePokemon(): Pokemon | undefined {
     return globalScene.getPokemonById(this.sourceId);
   }
 
@@ -617,7 +616,7 @@ export class NoCritTag extends SerializableArenaTag {
 
     globalScene.phaseManager.queueMessage(
       i18next.t("arenaTag:noCritOnRemove", {
-        pokemonNameWithAffix: getPokemonNameWithAffix(source ?? undefined),
+        pokemonNameWithAffix: getPokemonNameWithAffix(source),
         moveName: this.getMoveName(),
       }),
     );
@@ -1537,7 +1536,7 @@ export class SuppressAbilitiesTag extends SerializableArenaTag {
     }
   }
 
-  public override onOverlap(_arena: Arena, source: Pokemon | null): void {
+  public override onOverlap(_arena: Arena, source: Pokemon | undefined): void {
     (this as Mutable<this>).sourceCount++;
     this.playActivationMessage(source);
   }
@@ -1580,7 +1579,7 @@ export class SuppressAbilitiesTag extends SerializableArenaTag {
     return this.sourceCount > 1;
   }
 
-  private playActivationMessage(pokemon: Pokemon | null) {
+  private playActivationMessage(pokemon: Pokemon | undefined) {
     if (pokemon) {
       globalScene.phaseManager.queueMessage(
         i18next.t("arenaTag:neutralizingGasOnAdd", {

src/data/battler-tags.ts

@@ -198,7 +198,7 @@ export class BattlerTag implements BaseBattlerTag {
    * Helper function that retrieves the source Pokemon object
    * @returns The source {@linkcode Pokemon}, or `null` if none is found
    */
-  public getSourcePokemon(): Pokemon | null {
+  public getSourcePokemon(): Pokemon | undefined {
     return globalScene.getPokemonById(this.sourceId);
   }
 }
@@ -968,7 +968,7 @@ export class InfatuatedTag extends SerializableBattlerTag {
     phaseManager.queueMessage(
       i18next.t("battlerTags:infatuatedLapse", {
         pokemonNameWithAffix: getPokemonNameWithAffix(pokemon),
-        sourcePokemonName: getPokemonNameWithAffix(globalScene.getPokemonById(this.sourceId!) ?? undefined), // TODO: is that bang correct?
+        sourcePokemonName: getPokemonNameWithAffix(this.getSourcePokemon()),
       }),
     );
     phaseManager.unshiftNew("CommonAnimPhase", pokemon.getBattlerIndex(), undefined, CommonAnim.ATTRACT);

src/modifier/modifier.ts

@@ -728,7 +728,7 @@ export abstract class PokemonHeldItemModifier extends PersistentModifier {
   }
 
   getPokemon(): Pokemon | undefined {
-    return globalScene.getPokemonById(this.pokemonId) ?? undefined;
+    return globalScene.getPokemonById(this.pokemonId);
   }
 
   getScoreMultiplier(): number {

src/phases/move-effect-phase.ts

@@ -645,13 +645,18 @@ export class MoveEffectPhase extends PokemonPhase {
     return move.getAttrs("HitsTagAttr").some(hta => hta.tagType === semiInvulnerableTag.tagType);
   }
 
-  /** @returns The {@linkcode Pokemon} using this phase's invoked move */
-  public getUserPokemon(): Pokemon | null {
+  /**
+   * @todo Investigate why this doesn't use `BattlerIndex`
+   * @returns The {@linkcode Pokemon} using this phase's invoked move
+   */
+  public getUserPokemon(): Pokemon | undefined {
     // TODO: Make this purely a battler index
     if (this.battlerIndex > BattlerIndex.ENEMY_2) {
       return globalScene.getPokemonById(this.battlerIndex);
     }
-    return (this.player ? globalScene.getPlayerField() : globalScene.getEnemyField())[this.fieldIndex];
+    // TODO: Figure out why this uses `fieldIndex` instead of `BattlerIndex`
+    // TODO: Remove `?? undefined` once field pokemon getters are made sane
+    return (this.player ? globalScene.getPlayerField() : globalScene.getEnemyField())[this.fieldIndex] ?? undefined;
   }
 
   /**

src/phases/obtain-status-effect-phase.ts

@@ -23,6 +23,7 @@ export class ObtainStatusEffectPhase extends PokemonPhase {
    * @param sourceText - The text to show for the source of the status effect, if any; default `null`.
    * @param statusMessage - A string containing text to be displayed upon status setting;
    * defaults to normal key for status if empty or omitted.
+   * @todo stop passing `null` to the phase
    */
   constructor(
     battlerIndex: BattlerIndex,

src/phases/pokemon-phase.ts

@@ -9,7 +9,9 @@ export abstract class PokemonPhase extends FieldPhase {
    * TODO: Make this either use IDs or `BattlerIndex`es, not a weird mix of both
    */
   protected battlerIndex: BattlerIndex | number;
+  // TODO: Why is this needed?
   public player: boolean;
+  /** @todo Remove in favor of `battlerIndex` pleas for fuck's sake */
   public fieldIndex: number;
 
   constructor(battlerIndex?: BattlerIndex | number) {
@@ -32,10 +34,11 @@ export abstract class PokemonPhase extends FieldPhase {
     this.fieldIndex = battlerIndex % 2;
   }
 
+  // TODO: This should have `undefined` in its signature
   getPokemon(): Pokemon {
     if (this.battlerIndex > BattlerIndex.ENEMY_2) {
-      return globalScene.getPokemonById(this.battlerIndex)!; //TODO: is this bang correct?
+      return globalScene.getPokemonById(this.battlerIndex)!;
     }
-    return globalScene.getField()[this.battlerIndex]!; //TODO: is this bang correct?
+    return globalScene.getField()[this.battlerIndex]!;
   }
 }

test/battler-tags/substitute.test.ts

@@ -49,7 +49,7 @@ describe("BattlerTag - SubstituteTag", () => {
 
       vi.spyOn(messages, "getPokemonNameWithAffix").mockReturnValue("");
       vi.spyOn(mockPokemon.scene as BattleScene, "getPokemonById").mockImplementation(pokemonId =>
-        mockPokemon.id === pokemonId ? mockPokemon : null,
+        mockPokemon.id === pokemonId ? mockPokemon : undefined,
       );
     });