Applied PR reviews

src/battle-scene.ts

@@ -875,7 +875,7 @@ export default class BattleScene extends SceneBase {
    * Returns an array of Pokemon on both sides of the battle - player first, then enemy.
    * Does not actually check if the pokemon are on the field or not, and always has length 4 regardless of battle type.
    * @param activeOnly - Whether to consider only active pokemon (as described by {@linkcode Pokemon.isActive()}); default `false`.
-   * If `true`, will also elide all `null` values from the array.
+   * If `true`, will also remove all `null` values from the array.
    * @returns An array of {@linkcode Pokemon}, as described above.
    */
   public getField(activeOnly = false): Pokemon[] {
@@ -3354,6 +3354,8 @@ export default class BattleScene extends SceneBase {
     return (player ? this.modifiers : this.enemyModifiers).filter((m): m is T => m instanceof modifierType);
   }
 
+  // TODO: Add overload signature to `findModifier` and `findModifiers` for functions of the form `(x): X is T`
+
   /**
    * Get all of the modifiers that pass the `modifierFilter` function
    * @param modifierFilter - The function used to filter a target's modifiers

src/data/abilities/ability.ts

@@ -1202,7 +1202,10 @@ export class FieldPreventExplosiveMovesAbAttr extends AbAttr {
 export class FieldMultiplyStatAbAttr extends AbAttr {
   private stat: Stat;
   private multiplier: number;
-  /** Whether this ability can stack with others of the same type for this stat; default `false` */
+  /**
+   * Whether this ability can stack with others of the same type for this stat.
+   * @defaultValue `false`
+   */
   private canStack: boolean;
 
   constructor(stat: Stat, multiplier: number, canStack = false) {
@@ -1214,8 +1217,12 @@ export class FieldMultiplyStatAbAttr extends AbAttr {
   }
 
   canApplyFieldStat(_pokemon: Pokemon, _passive: boolean, _simulated: boolean, stat: Stat, _statValue: NumberHolder, checkedPokemon: Pokemon, hasApplied: BooleanHolder, _args: any[]): boolean {
-    return (this.canStack || !hasApplied.value)
+    return (
-      && this.stat === stat && checkedPokemon.getAbilityAttrs(FieldMultiplyStatAbAttr).every(attr => (attr as FieldMultiplyStatAbAttr).stat !== stat);
+      (this.canStack || !hasApplied.value)
+      && this.stat === stat
+      // targets with the same stat-changing ability as this are unaffected
+      && checkedPokemon.getAbilityAttrs(FieldMultiplyStatAbAttr).every(attr => attr.stat !== stat)
+    );
   }
 
   /**
@@ -1341,37 +1348,35 @@ export class PokemonTypeChangeAbAttr extends PreAttackAbAttr {
 /**
  * Class for abilities that add additional strikes to single-target moves.
  * Used by {@linkcode Moves.PARENTAL_BOND | Parental Bond}.
+ *
+ * @param damageMultiplier - The damage multiplier for the added strike, relative to the first.
 */
 export class AddSecondStrikeAbAttr extends PreAttackAbAttr {
   private damageMultiplier: number;
-
-  /**
-   * @param damageMultiplier - The damage multiplier for the added strike, relative to the first.
-  */
  constructor(damageMultiplier: number) {
     super(false);
 
     this.damageMultiplier = damageMultiplier;
   }
 
-  override canApplyPreAttack(pokemon: Pokemon, passive: boolean, simulated: boolean, defender: Pokemon | null, move: Move, args: any[]): boolean {
+  override canApplyPreAttack(pokemon: Pokemon, _passive: boolean, _simulated: boolean, _defender: Pokemon | null, move: Move, _args: [NumberHolder?, NumberHolder?]): boolean {
     return move.canBeMultiStrikeEnhanced(pokemon, true);
   }
 
   /**
-   * Applies the ability attribute by increasing hit count
+   * Increases the move's hit count or modifies the move's damage.
    * @param pokemon - The {@linkcode Pokemon} using the move
    * @param passive - Unused
    * @param defender - Unused
    * @param move - The {@linkcode Move} being used
-   * @param args:
+   * @param args -
    * - `[0]` - A {@linkcode NumberHolder} holding the move's current strike count
    * - `[1]` - A {@linkcode NumberHolder} holding the current strike's damage multiplier.
    */
-  // TODO: Why can these be nullish?
+  // TODO: Review if these args can be undefined when called (and remove the ? if not)
-  override applyPreAttack(pokemon: Pokemon, passive: boolean, simulated: boolean, defender: Pokemon, move: Move, args: any[]): void {
+  override applyPreAttack(pokemon: Pokemon, _passive: boolean, _simulated: boolean, _defender: Pokemon, _move: Move, args: [NumberHolder?, NumberHolder?]): void {
-    const hitCount = args[0] as NumberHolder;
+    const hitCount = args[0];
-    const multiplier = args[1] as NumberHolder;
+    const multiplier = args[1];
     if (hitCount?.value) {
       hitCount.value += 1;
     }
@@ -3147,10 +3152,11 @@ export class ProtectStatAbAttr extends PreStatStageChangeAbAttr {
 }
 
 /**
- * Attribute to apply confusion to the target whenever the user
+ * Attribute to apply confusion to the target whenever the user directly statuses them with a move.
- * directly statuses them with a move.
+ *
  * Used by {@linkcode Abilities.POISON_PUPPETEER}
- * Called in {@linkcode StatusEffectAttr}.
+ *
+ * Called in {@linkcode StatusEffectAttr}
  * @extends PostAttackAbAttr
  * @see {@linkcode applyPostAttack}
  */
@@ -3338,8 +3344,8 @@ export class ConditionalUserFieldProtectStatAbAttr extends PreStatStageChangeAbA
    * @param simulated - Unused
    * @param stat - The {@linkcode Stat} being affected
    * @param cancelled - {@linkcode BooleanHolder} containing whether the stat change was already prevented
-   * @param args `[0]` - The {@linkcode Pokemon} recieving the stat change
+   * @param args - `[0]`: The {@linkcode Pokemon} receiving the stat change
-   * @returns  `true` if the ability can be applied
+   * @returns `true` if the ability can be applied
   */
   override canApplyPreStatStageChange(pokemon: Pokemon, passive: boolean, simulated: boolean, stat: BattleStat, cancelled: BooleanHolder, args: [Pokemon]): boolean {
     const target = args[0];
@@ -3534,9 +3540,9 @@ export class ConditionalCritAbAttr extends AbAttr {
    * @param simulated - Unused
    * @param cancelled - Unused
    * @param args -
-   * - [0] A {@linkcode BooleanHolder} containing whether critical hits are guaranteed.
+   * - `[0]` A {@linkcode BooleanHolder} containing whether critical hits are guaranteed.
-   * - [1] The {@linkcode Pokemon} being targeted (unused)
+   * - `[1]` The {@linkcode Pokemon} being targeted (unused)
-   * - [2] The {@linkcode Move} used by the ability holder (unused)
+   * - `[2]` The {@linkcode Move} used by the ability holder (unused)
    */
   override apply(pokemon: Pokemon, passive: boolean, simulated: boolean, cancelled: BooleanHolder, args: [BooleanHolder, Pokemon, Move, ...any]): void {
     (args[0] as BooleanHolder).value = true;

src/data/arena-tag.ts

@@ -69,9 +69,9 @@ export abstract class ArenaTag {
 
   /**
    * Trigger this {@linkcode ArenaTag}'s effect, reducing its duration as applicable.
-   * Will ignore duration of all tags with durations alwrea
+   * Will ignore durations of all tags with durations `<=0`.
    * @param _arena - The {@linkcode Arena} at the moment the tag is being lapsed.
-   * Unused by default but can be used by super classes.
+   * Unused by default but can be used by sub-classes.
    * @returns `true` if this tag should be kept; `false` if it should be removed.
    */
   lapse(_arena: Arena): boolean {
@@ -881,7 +881,6 @@ class ToxicSpikesTag extends ArenaTrapTag {
  * Arena Tag class for delayed attacks, such as {@linkcode Moves.FUTURE_SIGHT} or {@linkcode Moves.DOOM_DESIRE}.
  * Delays the attack's effect by a set amount of turns, usually 3 (including the turn the move is used),
  * and deals damage after the turn count is reached.
-  // TODO: Add class for tags that can have multiple instances up and edit `arena.addTag` appropriately
  */
 export class DelayedAttackTag extends ArenaTag {
   public targetIndex: BattlerIndex;

src/data/moves/move.ts

@@ -201,7 +201,7 @@ export default class Move implements Localizable {
   /**
    * Check if a move has an attribute that matches `attrType`
    * @param attrType - The constructor of a {@linkcode MoveAttr}
-   * @returns Whether if the move has an attribute that is/extends `attrType`
+   * @returns Whether this move has an attribute matching `attrType`
    */
   hasAttr<T extends MoveAttr>(attrType: Constructor<T>): boolean {
     return this.attrs.some((attr) => attr instanceof attrType);
@@ -210,10 +210,12 @@ export default class Move implements Localizable {
   /**
    * Find the first attribute that matches a given predicate function.
    * @param attrPredicate - The predicate function to search `MoveAttr`s by.
-   * @returns The first {@linkcode MoveAttr} for which `attrPredicate` returns a value coercible to the boolean value `true`.
+   * @returns The first {@linkcode MoveAttr} for which `attrPredicate` returns `true`.
    */
   findAttr(attrPredicate: (attr: MoveAttr) => boolean): MoveAttr {
-    return this.attrs.find(attrPredicate)!; // TODO: is the bang correct?
+    // TODO: Remove bang and make return type `MoveAttr | undefined`,
+    // as well as add overload for functions of type `x is T`
+    return this.attrs.find(attrPredicate)!;
   }
 
   /**
@@ -241,7 +243,7 @@ export default class Move implements Localizable {
    * Adds a new MoveAttr to this move (appends to the attr array).
    * If the MoveAttr also comes with a condition, it is added to its {@linkcode MoveCondition} array.
    *
-   * Almost identical to {@linkcode attr}, except taking already instantized {@linkcode MoveAttr} object
+   * Similar to {@linkcode attr}, except this takes an already instantiated {@linkcode MoveAttr} object
    * as opposed to a constructor and its arguments.
    * @param attrAdd - The {@linkcode MoveAttr} to add
    * @returns `this`
@@ -386,8 +388,8 @@ export default class Move implements Localizable {
 
   /**
    * Mark a move as having one or more edge cases.
-   * The move may lack certain niche interactions with other moves/abilities, but still functions
+   * The move may lack certain niche interactions with other moves/abilities,
-   * as intended in most cases.
+   * but still functions as intended in most cases.
    *
    * When using this, **make sure to document the edge case** (or else this becomes pointless).
    * @returns `this`
@@ -397,8 +399,8 @@ export default class Move implements Localizable {
   }
 
   /**
-   * Mark a move as partially implemented.
+   * Mark this move as partially implemented.
-   * Partial moves are expected to have their core functionality implemented, but may lack
+   * Partial moves are expected to have some core functionality implemented, but may lack
    * certain notable features or interactions with other moves or abilities.
    * @returns `this`
    */
@@ -408,9 +410,9 @@ export default class Move implements Localizable {
   }
 
   /**
-   * Mark a move as unimplemented.
+   * Mark this move as unimplemented.
    * Unimplemented moves are ones which have _none_ of their basic functionality enabled,
-   * and are effectively barred from use by the AI.
+   * and will fail upon use.
    * @returns `this`
    */
   unimplemented(): this {
@@ -976,10 +978,8 @@ export class AttackMove extends Move {
   constructor(id: Moves, type: PokemonType, category: MoveCategory, power: number, accuracy: number, pp: number, chance: number, priority: number, generation: number) {
     super(id, type, category, MoveTarget.NEAR_OTHER, power, accuracy, pp, chance, priority, generation);
 
-    /**
+    // > All damaging Fire-type moves can... thaw a frozen target, regardless of whether or not they have a chance to burn.
-     * {@link https://bulbapedia.bulbagarden.net/wiki/Freeze_(status_condition)}
+    // - https://bulbapedia.bulbagarden.net/wiki/Freeze_(status_condition)
-     * All damaging Fire-type moves can thaw a frozen target, regardless of whether or not they have a chance to burn
-     */
     if (this.type === PokemonType.FIRE) {
       this.addAttr(new HealStatusEffectAttr(false, StatusEffect.FREEZE));
     }
@@ -2865,6 +2865,8 @@ export class HealStatusEffectAttr extends MoveEffectAttr {
  * Attribute to add the {@linkcode BattlerTagType.BYPASS_SLEEP | BYPASS_SLEEP Battler Tag} for 1 turn to the user before move use.
  * Used by {@linkcode Moves.SNORE} and {@linkcode Moves.SLEEP_TALK}.
  */
+// TODO: Should this use a battler tag?
+// TODO: Give this `userSleptOrComatoseCondition`
 export class BypassSleepAttr extends MoveAttr {
   apply(user: Pokemon, target: Pokemon, move: Move, args: any[]): boolean {
     if (user.status?.effect === StatusEffect.SLEEP) {
@@ -2882,7 +2884,7 @@ export class BypassSleepAttr extends MoveAttr {
    * @param move
    */
   getUserBenefitScore(user: Pokemon, target: Pokemon, move: Move): number {
-    return user.status && user.status.effect === StatusEffect.SLEEP ? 200 : -10;
+    return user.status?.effect === StatusEffect.SLEEP ? 200 : -10;
   }
 }
 

src/data/mystery-encounters/utils/encounter-pokemon-utils.ts

@@ -404,7 +404,7 @@ export async function applyModifierTypeToPlayerPokemon(
   // Check if the Pokemon has max stacks of that item already
   const modifier = modType.newModifier(pokemon);
   const existing = globalScene.findModifier(
-    m =>
+    (m): m is PokemonHeldItemModifier =>
       m instanceof PokemonHeldItemModifier &&
       m.type.id === modType.id &&
       m.pokemonId === pokemon.id &&

src/field/arena.ts

@@ -691,7 +691,6 @@ export class Arena {
     targetIndex?: BattlerIndex,
   ): boolean {
     const existingTag = this.getTagOnSide(tagType, side);
-    // TODO: Change this for future sight
     if (existingTag) {
       existingTag.onOverlap(this, globalScene.getPokemonById(sourceId));
 
@@ -704,7 +703,6 @@ export class Arena {
     }
 
     // creates a new tag object
-    // TODO: check if we can remove the `|| 0` since turn count should never be nullish
     const newTag = getArenaTag(tagType, turnCount || 0, sourceMove, sourceId, targetIndex, side);
     if (newTag) {
       newTag.onAdd(this, quiet);

src/field/pokemon.ts

@@ -586,7 +586,7 @@ export default abstract class Pokemon extends Phaser.GameObjects.Container {
    * @returns The {@linkcode PokeballType} that will be shown when this Pokemon is sent out into battle.
    */
   getPokeball(useIllusion = false): PokeballType {
-    return useIllusion && this.summonData?.illusion
+    return useIllusion && this.summonData.illusion
       ? this.summonData.illusion.pokeball
       : this.pokeball;
     }
@@ -645,7 +645,7 @@ export default abstract class Pokemon extends Phaser.GameObjects.Container {
 
   /**
    * Checks if a pokemon is fainted (ie: its `hp <= 0`).
-   * Usually should not be called directly in favor of consulting {@linkcode isAllowedInBattle()}.
+   * Usually should not be called directly in favor of calling {@linkcode isAllowedInBattle()}.
    * @param checkStatus - Whether to also check that the pokemon's status is {@linkcode StatusEffect.FAINT}; default `false`
    * @returns Whether this Pokemon is fainted, as described above.
    */
@@ -2270,9 +2270,9 @@ export default abstract class Pokemon extends Phaser.GameObjects.Container {
   /**
    * Get this Pokemon's non-passive {@linkcode Ability}, factoring in fusions, overrides and ability-changing effects.
 
-   * Should rarely be called directky in favor of {@linkcode hasAbility} or {@linkcode hasAbilityWithAttr},
+   * Should rarely be called directly in favor of {@linkcode hasAbility} or {@linkcode hasAbilityWithAttr},
    * both of which check both ability slots and account for suppression.
-   * @see {@linkcode hasAbility}and {@linkcode hasAbilityWithAttr} Intended ways to check abilities in most cases
+   * @see {@linkcode hasAbility} and {@linkcode hasAbilityWithAttr} are the intended ways to check abilities in most cases
    * @param ignoreOverride - Whether to ignore any overrides caused by {@linkcode Moves.TRANSFORM | Transform}; default `false`
    * @returns The non-passive {@linkcode Ability} of this Pokemon.
    */
@@ -6424,7 +6424,7 @@ export default abstract class Pokemon extends Phaser.GameObjects.Container {
   /**
    * Reduces one of this Pokemon's held item stacks by 1, removing it if applicable.
    * Does nothing if this Pokemon is somehow not the owner of the held item.
-   * @param heldItem The item stack to be reduced.
+   * @param heldItem - The item stack to be reduced.
    * @param forBattle - Whether to trigger in-battle effects (such as Unburden) after losing the item. Default: `true`
    * Should be `false` for all item loss occurring outside of battle (MEs, etc.).
    * @returns Whether the item was removed successfully.

src/modifier/modifier.ts

@@ -62,25 +62,11 @@ const iconOverflowIndex = 24;
 export const modifierSortFunc = (a: Modifier, b: Modifier): number => {
   const itemNameMatch = a.type.name.localeCompare(b.type.name);
   const typeNameMatch = a.constructor.name.localeCompare(b.constructor.name);
-  const aId = a instanceof PokemonHeldItemModifier && a.pokemonId ? a.pokemonId : 4294967295;
+  const aId = a instanceof PokemonHeldItemModifier ? a.pokemonId : -1;
-  const bId = b instanceof PokemonHeldItemModifier && b.pokemonId ? b.pokemonId : 4294967295;
+  const bId = b instanceof PokemonHeldItemModifier ? b.pokemonId : -1;
 
-  // First sort by pokemonID
+  // First sort by pokemon ID, then by item type and then name
-  if (aId < bId) {
+  return aId - bId || typeNameMatch || itemNameMatch;
-    return 1;
-  }
-  if (aId > bId) {
-    return -1;
-  }
-  if (aId === bId) {
-    //Then sort by item type
-    if (typeNameMatch === 0) {
-      return itemNameMatch;
-      //Finally sort by item name
-    }
-    return typeNameMatch;
-  }
-  return 0;
 };
 
 export class ModifierBar extends Phaser.GameObjects.Container {