Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dex*: Consistent field init order in constructors #10608

Merged
merged 7 commits into from
Oct 31, 2024
+35 −33
Merged

Dex*: Consistent field init order in constructors #10608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28008bb
BasicEffect: consistent field init in ctor
larry-the-table-guy Oct 12, 2024
f8045c6
Dex*: consistent field init in ctors
larry-the-table-guy Oct 12, 2024
15e7ead
Merge branch 'master' into sim/consistent-field-ordering
larry-the-table-guy Oct 14, 2024
97d0282
Sim: Make `BasicEffect` an abstract class
larry-the-table-guy Oct 23, 2024
5a23223
Simplify effect.exists
Zarel Oct 29, 2024
8ff59e5
Rename `assignNewFields` to `assignMissingFields`
larry-the-table-guy Oct 30, 2024
744bf7c
Simplify `assignMissingFields`
larry-the-table-guy Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sim/dex-abilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {PokemonEventMethods, ConditionData} from './dex-conditions';
import {BasicEffect, toID} from './dex-data';
import {assignMissingFields, BasicEffect, toID} from './dex-data';
import {Utils} from '../lib';

interface AbilityEventMethods {
Expand Down Expand Up @@ -64,6 +64,7 @@ export class Ability extends BasicEffect implements Readonly<BasicEffect> {
this.gen = 3;
}
}
assignMissingFields(this, data);
}
}

Expand Down
5 changes: 2 additions & 3 deletions sim/dex-conditions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Utils} from '../lib';
import {BasicEffect, toID} from './dex-data';
import {assignMissingFields, BasicEffect, toID} from './dex-data';
import type {SecondaryEffect, MoveEventMethods} from './dex-moves';

export interface EventMethods {
Expand Down Expand Up @@ -628,9 +628,8 @@ export class Condition extends BasicEffect implements

constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;
this.effectType = (['Weather', 'Status'].includes(data.effectType) ? data.effectType : 'Condition');
assignMissingFields(this, data);
}
}

Expand Down
29 changes: 17 additions & 12 deletions sim/dex-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,19 @@ export function toID(text: any): ID {
/* eslint-enable @typescript-eslint/prefer-optional-chain */
}

export class BasicEffect implements EffectData {
/**
* Like Object.assign but only assigns fields missing from self.
* Facilitates consistent field ordering in constructors.
* Modifies self in-place.
*/
export function assignMissingFields(self: AnyObject, data: AnyObject) {
for (const k in data) {
if (k in self) continue;
self[k] = data[k];
}
}

export abstract class BasicEffect implements EffectData {
/**
* ID. This will be a lowercase version of the name with all the
* non-alphanumeric characters removed. So, for instance, "Mr. Mime"
Expand Down Expand Up @@ -102,14 +114,11 @@ export class BasicEffect implements EffectData {
sourceEffect: string;

constructor(data: AnyObject) {
this.exists = true;
Object.assign(this, data);

this.name = Utils.getString(data.name).trim();
this.id = data.realMove ? toID(data.realMove) : toID(this.name); // Hidden Power hack
this.fullname = Utils.getString(data.fullname) || this.name;
this.effectType = Utils.getString(data.effectType) as EffectType || 'Condition';
this.exists = !!(this.exists && this.id);
this.exists = data.exists ?? !!this.id;
this.num = data.num || 0;
this.gen = data.gen || 0;
Comment on lines 117 to 123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that if you want the common prefix as long as possible, fullname and num should be below gen, to match TypeInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to eventually explore re-ordering fields. I suspect it will be a smaller effect than this PR, but I won't rule it out yet.
My gut says that it matters most within a class - previously, different instances of Species could have wildly different field orders. Same for other classes.
Outside of micro-benchmarks, it gets very hard to reason about what works and what doesn't, which is why I try to make sure I have measurements to justify code changes.

this.shortDesc = data.shortDesc || '';
Expand All @@ -134,14 +143,12 @@ export class Nature extends BasicEffect implements Readonly<BasicEffect & Nature
readonly minus?: StatIDExceptHP;
constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;

this.fullname = `nature: ${this.name}`;
this.effectType = 'Nature';
this.gen = 3;
this.plus = data.plus || undefined;
this.minus = data.minus || undefined;
assignMissingFields(this, data);
}
}

Expand Down Expand Up @@ -257,18 +264,16 @@ export class TypeInfo implements Readonly<TypeData> {
readonly HPdvs: SparseStatsTable;

constructor(data: AnyObject) {
this.exists = true;
Object.assign(this, data);

this.name = data.name;
this.id = data.id;
this.effectType = Utils.getString(data.effectType) as TypeInfoEffectType || 'Type';
this.exists = !!(this.exists && this.id);
this.exists = data.exists ?? !!this.id;
this.gen = data.gen || 0;
this.isNonstandard = data.isNonstandard || null;
this.damageTaken = data.damageTaken || {};
this.HPivs = data.HPivs || {};
this.HPdvs = data.HPdvs || {};
assignMissingFields(this, data);
}

toString() {
Expand Down
5 changes: 2 additions & 3 deletions sim/dex-formats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Utils} from '../lib';
import {toID, BasicEffect} from './dex-data';
import {assignMissingFields, toID, BasicEffect} from './dex-data';
import {EventMethods} from './dex-conditions';
import {SpeciesData} from './dex-species';
import {Tags} from '../data/tags';
Expand Down Expand Up @@ -478,8 +478,6 @@ export class Format extends BasicEffect implements Readonly<BasicEffect> {

constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;

this.mod = Utils.getString(data.mod) || 'gen9';
this.effectType = Utils.getString(data.effectType) as FormatEffectType || 'Format';
Expand All @@ -496,6 +494,7 @@ export class Format extends BasicEffect implements Readonly<BasicEffect> {
this.onBegin = data.onBegin || undefined;
this.noLog = !!data.noLog;
this.playerCount = (this.gameType === 'multi' || this.gameType === 'freeforall' ? 4 : 2);
assignMissingFields(this, data);
}
}

Expand Down
6 changes: 3 additions & 3 deletions sim/dex-items.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {PokemonEventMethods, ConditionData} from './dex-conditions';
import {BasicEffect, toID} from './dex-data';
import {assignMissingFields, BasicEffect, toID} from './dex-data';
import {Utils} from '../lib';

interface FlingData {
Expand Down Expand Up @@ -107,8 +107,6 @@ export class Item extends BasicEffect implements Readonly<BasicEffect> {

constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;

this.fullname = `item: ${this.name}`;
this.effectType = 'Item';
Expand Down Expand Up @@ -152,6 +150,8 @@ export class Item extends BasicEffect implements Readonly<BasicEffect> {
if (this.onDrive) this.fling = {basePower: 70};
if (this.megaStone) this.fling = {basePower: 80};
if (this.onMemory) this.fling = {basePower: 50};

assignMissingFields(this, data);
}
}

Expand Down
11 changes: 5 additions & 6 deletions sim/dex-moves.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {Utils} from '../lib';
import type {ConditionData} from './dex-conditions';
import {BasicEffect, toID} from './dex-data';
import {assignMissingFields, BasicEffect, toID} from './dex-data';

/**
* Describes the acceptable target(s) of a move.
Expand Down Expand Up @@ -481,8 +481,6 @@ export class DataMove extends BasicEffect implements Readonly<BasicEffect & Move

constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;

this.fullname = `move: ${this.name}`;
this.effectType = 'Move';
Expand Down Expand Up @@ -520,7 +518,7 @@ export class DataMove extends BasicEffect implements Readonly<BasicEffect & Move
this.forceSTAB = !!data.forceSTAB;
this.volatileStatus = typeof data.volatileStatus === 'string' ? (data.volatileStatus as ID) : undefined;

if (this.category !== 'Status' && !this.maxMove && this.id !== 'struggle') {
if (this.category !== 'Status' && !data.maxMove && this.id !== 'struggle') {
this.maxMove = {basePower: 1};
if (this.isMax || this.isZ) {
// already initialized to 1
Expand Down Expand Up @@ -560,10 +558,10 @@ export class DataMove extends BasicEffect implements Readonly<BasicEffect & Move
}
}
}
if (this.category !== 'Status' && !this.zMove && !this.isZ && !this.isMax && this.id !== 'struggle') {
if (this.category !== 'Status' && !data.zMove && !this.isZ && !this.isMax && this.id !== 'struggle') {
let basePower = this.basePower;
this.zMove = {};
if (Array.isArray(this.multihit)) basePower *= 3;
if (Array.isArray(data.multihit)) basePower *= 3;
if (!basePower) {
this.zMove.basePower = 100;
} else if (basePower >= 140) {
Expand Down Expand Up @@ -611,6 +609,7 @@ export class DataMove extends BasicEffect implements Readonly<BasicEffect & Move
this.gen = 1;
}
}
assignMissingFields(this, data);
}
}

Expand Down
9 changes: 4 additions & 5 deletions sim/dex-species.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {assignMissingFields, BasicEffect, toID} from './dex-data';
import {Utils} from '../lib';
import {toID, BasicEffect} from './dex-data';

interface SpeciesAbility {
0: string;
Expand Down Expand Up @@ -273,8 +273,6 @@ export class Species extends BasicEffect implements Readonly<BasicEffect & Speci

constructor(data: AnyObject) {
super(data);
// eslint-disable-next-line @typescript-eslint/no-this-alias
data = this;

this.fullname = `pokemon: ${data.name}`;
this.effectType = 'Pokemon';
Expand Down Expand Up @@ -306,7 +304,7 @@ export class Species extends BasicEffect implements Readonly<BasicEffect & Speci
this.gender === 'N' ? {M: 0, F: 0} :
{M: 0.5, F: 0.5});
this.requiredItem = data.requiredItem || undefined;
this.requiredItems = this.requiredItems || (this.requiredItem ? [this.requiredItem] : undefined);
this.requiredItems = data.requiredItems || (this.requiredItem ? [this.requiredItem] : undefined);
this.baseStats = data.baseStats || {hp: 0, atk: 0, def: 0, spa: 0, spd: 0, spe: 0};
this.bst = this.baseStats.hp + this.baseStats.atk + this.baseStats.def +
this.baseStats.spa + this.baseStats.spd + this.baseStats.spe;
Expand All @@ -325,8 +323,8 @@ export class Species extends BasicEffect implements Readonly<BasicEffect & Speci
this.battleOnly = data.battleOnly || (this.isMega ? this.baseSpecies : undefined);
this.changesFrom = data.changesFrom ||
(this.battleOnly !== this.baseSpecies ? this.battleOnly : this.baseSpecies);
if (Array.isArray(this.changesFrom)) this.changesFrom = this.changesFrom[0];
this.pokemonGoData = data.pokemonGoData || undefined;
if (Array.isArray(data.changesFrom)) this.changesFrom = data.changesFrom[0];

if (!this.gen && this.num >= 1) {
if (this.num >= 906 || this.forme.includes('Paldea')) {
Expand All @@ -353,6 +351,7 @@ export class Species extends BasicEffect implements Readonly<BasicEffect & Speci
this.gen = 1;
}
}
assignMissingFields(this, data);
}
}

Expand Down
Loading