smogon / pokemon-showdown

Pokémon battle simulator.
https://pokemonshowdown.com
MIT License
4.77k stars 2.79k forks source link

Code should be split up #8806

Closed BluezamX closed 1 year ago

BluezamX commented 2 years ago

A lot of the code within the project should be split up into separate methods with names like ResetVariables or something for battle.ts lines ~157 to ~234, etc. There are just a lot of files with >2000 lines and methods with >200 lines which should be split up into separate, smaller methods for readability, also allowing new contributors to help more easily.

There's also a lot of duplicate code - for example in battle.ts lines 487 - 494 and 672 - 679

Slayer95 commented 2 years ago

@BluezamX , the lines you point at are property definitions in the Battle class constructor. Keeping those definitions in the top level is standard practice. On the contrary, moving them out to a separate class would obscure the shape of the class for static analysis, which may even impact performance.

Looking at the same Battle#constructor(), however, I believe the following lines (238 to 274) may be moved away once someone figures out a fitting name and sends a PR.

The other lines you mention corresponds to error paths. Moving them away would consume one extra trace frame, which would cause loss of debugging information, possibly critical in the corresponding stack limit exceeded escenarios.

If you insist, however, I encourage you to send a PR, and it's possible the current maintainers are more supportive of your proposals. (I am an odd one in favoring stack traces against modularization of validators).

Slayer95 commented 2 years ago

More general discussion:

Please note that nowadays Pokémon Showdown simulates games according to pre-defined behaviors across 8 generations and up to 6 game modes for each generation, plus official and unofficial Other Metagames and mods.

For that purpose, it leverages a powerful event system. However, the the requirement to handle so many variants causes some routines to naturally end up being quite large. If only you had a look at our monolythic files of yesteryear! Another strategy followed by PS is dogfeeding our modding API into generation-specific mods, which alleviates that issue. However, since modding is basically monkey-patching, it results in the other issue you mention: duplication.

These many challenges have been tackled to some degree by @scheibo in his fork https://github.com/pkmn/ps . However, my information channels are limited and I am unaware of ongoing efforts to merge that work here.

It's my expectation, however, that a piecemeal approach through successive small and sound PRs would yield better results than a general issue.

Slayer95 commented 2 years ago

On the other hand, I am empathetic to first or sporadic contributors who try to edit the source code of the data/ files through the Github Web API but then Github hangs or straight up refuses to display the source code due to its sheer size.

I'm actually not a fan of this proposal, but I shall raise it anyway. What if data/(moves|abilities|items|learnsets|formats-data) were split alphabetically?

monsanto commented 1 year ago

No response to follow-ups in months so I am closing this.