Use "getter" pattern to derive current stats in Pokemon class

[ajhyndman]

I want to propose computing the current "real" stats for a pokemon at access time instead of constructor time. I think this might clean up some weird logic, and also make it easier to display the effect of stat boosts, items, and abilities on a pokemon's real stats.

This could look like changing .stats to .getStats() on the Pokemon class, or if we want to be clever, it could even be a property "getter" pattern.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

This would obviously require some refactoring in the calculation logic though, so I wanted to ask before investing any time in it.

[scheibo]

We don't do getters because we target old browsers and havent added a bundler/transpiler (see our tsconfig still outputs es3). You'll notice I already have a comment https://github.com/smogon/damage-calc/blob/master/calc/src/pokemon.ts#L111 because maxHP is another obvious getter. #328

Can you point out some specific cases where moving the time when stats are initialized would matter? I'd be fine with adding a getStats or getComputedStats or something method provided I understand the problem it would solve, but I'm working on several projects right now and can't quite remember the cases in this repo where its important

[ajhyndman]

Oh, wow. Do you still see usage from browsers older than IE 9? Anyway, it's not really that big of a difference to make it an explicit function call.

As mentioned above, what I want to do in my project is have a reliable way to update IVs, EVs, stat boosts, abilities or items on an existing Pokemon instance and get the correct real stats. I've resorted to creating a new Pokemon on each change for now, but it seems a little silly. It looks like that's what this calculator client does, too:

https://github.com/smogon/damage-calc/blob/c7b053c403c9c1758643f7acc72fafde68d1a367/src/js/shared_controls.js#L555

This is probably fine. It just felt like a bit of a trap when I stumbled on.

---

I was also thinking that it might create an opportunity to move some of the ability and item computation that "modifies stats" into the pokemon class. I guess I'm not certain how much of that is consistent across generations though.

[scheibo]

Oh, wow. Do you still see usage from browsers older than IE 9? Anyway, it's not really that big of a difference to make it an explicit function call.

No, and from looking at our own analytics and PS's we dont really see that traffic. Its an argument I've had with Zarel where we shouldnt be supporting browsers neither of userbases use, but because I plan to integrate the calc directly into the client their min version is our min version, and Zarel seems to like to support ancient browsers because 'he likes the challenge' or something.

As mentioned above, what I want to do in my project is have a reliable way to update IVs, EVs, stat boosts, abilities or items on an existing Pokemon instance and get the correct real stats. I've resorted to creating a new Pokemon on each change for now, but it seems a little silly. It looks like that's what this calculator client does, too:

Yeah, I was just going to refer you to what the client does. Why is it silly? There shouldn't be a lot of overhead to creating said object, nor is where its being used (in response to user changes) latency critical even if it were.

This is probably fine. It just felt like a bit of a trap when I stumbled on.

I actually wanted all of our classes to be immutable because it dramatically reduces the number of bugs, but I had to compromise which is why we .clone() our objects all the time. I had them immutable in my original rewrite, but the mechanics code mutates the Pokemon a lot for use in the Description and it was a pain to refactor. IMO, mutating the Pokemon is really an anti pattern and the fact that we do it internally in the mechanics is a legacy implementation detail which I'd like to get around to fixing.

[ajhyndman]

I'm onboard with immutable pokemon instances.

Maybe what I want then is an updatePokemon(pokemon: Pokemon): Pokemon function or a method like clone that takes overrides. There's not actually any pattern for taking an existing instance and creating a new one.

I wrote this helper, but it seems to me it only sort of works by accident that most of the properties of a pokemon overlap with the shape the constructor expects.

https://github.com/ajhyndman/visual-pokemon-calc/blob/master/src/util/misc.ts#L25-L26

[Zarel]

I also don't like getters in general; I think they make code less readable compared to just adding () after the variable name so it's clear that it's a function call.

[Zarel]

I wrote this helper, but it seems to me it only sort of works by accident that most of the properties of a pokemon overlap with the shape the constructor expects.

No, that's not a hack, the constructors intentionally support new Pokemon(pokemon) to clone.

[ajhyndman]

No, that's not a hack, the constructors intentionally support new Pokemon(pokemon) to clone.

Oh, that makes me feel better about relying on it. Wouldn't it make sense to make it explicit that the constructor accepts a Partial<Pokemon> instance then?

Relatedly, the pokemon's properties still end up copied twice in my solution. Once during my object spread, and then again internally in the constructor. What if .clone() accepted an overrides object? That was another thing I tried to do before realising it wasn't supported.

[Zarel]

I'm not sure if what the constructor accepts is a Partial<Pokemon>. It probably makes more sense that the constructor accepts a PokemonData, and Pokemon implements PokemonData.

I think some constructors accept an overrides object. Not all of them, though?

[scheibo]

~What you call PokemonData is called Species in this project (the raw data files JSON object). Pokemon implements PokemonData is the same crap we're trying to get away with Pokemon in the client (https://github.com/smogon/pokemon-showdown-client/pull/1479). If you want the Species information on the Pokemon instance, get it through the species field.~ EDIT: Rereading this Zarel its possible I misintrepreted what you meant by PokemonData and we're actually on the same page here, sorry.

I think most of the constructors accept overrides, but thats an overrides of the core data files (ie. Species here), not the fields of Pokemon. All (or almost all) of the constructors use an options param pattern to avoid having to specify too many fields when new-ing one up and thats basically where you provide the species fields (though you're hardly "overriding" them, they dont exist yet because youre literally constructing the object right then).

I would be down with the constructors Foo (like Pokemon) accepting Partial<Foo> & {overrides?: Partial<FooData>} explicitly, though I feel theyre already probably doing that already. The exception is computed values like stats here which is the one you care about, so you might need to make it a Partial<Exclude<Foo, 'stats'>> etc.

[Zarel]

Heh, this would probably be a lot harder to misinterpret if we did the Template -> Species rename everywhere.

You're right, though, I did mean Template implements TemplateData.

[Zarel]

I like that TemplateData is more explicit than Partial<Template>, though.

[Zarel]

After all, not all fields are optional. Some are required, and TemplateData specifies which ones.

[ajhyndman]

After poking at my project some more today, I realised what I really want to get out of this is to be able to show total stats (after all modifiers) to users. And it turns out that's quite difficult without reimplementing or pulling out internals of the existing calculator code.

Speed calculation is here, and requires field information: https://github.com/smogon/damage-calc/blob/master/calc/src/mechanics/util.ts#L38

A lot of item and ability logic is currently tied to other non-stat damage modifiers, here: https://github.com/smogon/damage-calc/blob/master/calc/src/mechanics/gen8.ts#L884

It's true that's only relevant to my project, but it doesn't seem ideal for us both to maintain separate copies of the logic, so I'd rather find a way to contribute, if you're open to it.

[scheibo]

I have an interested in in computed stats for a project I'm working on as well, and theres already several places where it is done (here, in the sim code and in battle tooltips on the client). Come to think of it, its possibly you just want https://github.com/smogon/pokemon-showdown-client/blob/master/src/battle-tooltips.ts#L827.

I dont really have much bandwidth to figure out a good way of unifying everything (or even just exposing this in a good way in the calc - remember, the item and ability logic is actually spread accross all the gen files, not just in the one file you linked) at this point, but I would accept a change which made some progress on this front

[ajhyndman]

Oh yeah, that looks like what I want to do! Cool, I'll have a think about it.

I think I can close this issue for now.