Contributing refactoring?

[punmechanic]

I don't quite have the knowledge of this project in order to be able to submit new features or bugfixes quite yet, but just reading through the code a lot of it is quite dense. For example, battle-engine.js is some 5k lines long and could use being broken up into more manageable parts. Additionally, it's quite clear that you're using ES6 at least on the server side but the classes you've got in there aren't actually using ES6 syntax which means you have to wrap the entire class in an IIFE which looks a bit dodgy.

Would it be okay to submit some refactoring improvements in a PR or are bugfixes/features the main priority? (As long as it is actually refactoring and not breaking things)

[ascriptmaster]

If you really think you can help by refactoring then you are free to try

[Zarel]

We started writing this code back in 2011, way before Node supported ES6. We actually only started requiring versions of Node with good ES6 support around five seconds ago: https://github.com/Zarel/Pokemon-Showdown/commit/9db1d67c36b7c3459959d6c5f71dfd383e58c1e7

So yeah, a lot of the code's outdated. We use real classes in some places, we use old patterns in other places.

ladders.js is probably our most modern code? https://github.com/Zarel/Pokemon-Showdown/blob/master/ladders.js - it might be a good place to reference how I probably want the code to look like.

I would probably recommend doing changes a little at a time, and ask in the Development chatroom about specific changes before doing them, since sometimes there's a reason we don't do something, and I wouldn't want you to spend too much time on something we don't accept.

But if you submit a refactor, we will definitely either accept it or discuss why we don't think it's the best way to do things.

[Zarel]

I think an important caveat when it comes to refactoring is that we generally prioritize performance to readability, especially in inner loops.

[Zarel]

But if you just want to refactor prototype-based code to ES6 classes, I am 100% behind you; you don't need to ask.

[punmechanic]

Cool stuff. I attempted to look at battle engine and almost cried given how complex the file is. One of the first aims I think would be to split some files up but I see you guys don't actually have a dedicated src folder - everything is in the project root. Is this intended?

[Zarel]

Splitting up battle-engine - sure, how would you split it up?

Honestly, most of the reason we don't have a src folder is because I'm not very aware how standard that is and why it's done. Do you have any links to documentation explaining that standard?

[Zarel]

A lot of weird names for the relevant files is because I didn't want to name anything battle.js because I didn't want to have the same name for a server file as a client file, but honestly it might have been better if I did.

Proposed renames:

• simulator.js -> room-game-battle.js
• battle-engine.js -> sim.js, sim-pokemon.js, sim-side.js etc, however you decide to split it up

[kotarou3]

Arguably I'd put the sim-*.js stuff into its own directory

[Zarel]

I think a directory may be overkill for like three files, tops.

[punmechanic]

@Zarel Well, splitting each class into it's own file might be a start; doesn't even need to involve directories, but when you start having one responsibility per file you start to want to have directories for separation because your file count balloons.

This is how things are done in many languages (C#, Java for a start have a standard one type per file rule, as does Rust). There isn't necessarily an official standard for it, but many many libraries I see do this and every project I work on does this just to make organisation easier.

[Zarel]

I'm not sure file count ballooning is necessarily a good thing? What are the advantages and disadvantages?

Mostly I've split huge files but prefer moderately-sized files to tiny files. At least with my workflow, having a lot of files and having to dig through lots of directories to find them seems disadvantageous, and I'm not clear on what the advantages are.

The same thing applies to directory arrangement. I like having all my files right there, instead of having to open a src/ directory.

[Zarel]

I've spent most of my professional life on Pokémon Showdown, so I'm not too familiar with other codebases. My main other experiences with large projects:

• Warzone 2100, in C, moderate number of moderate-sized files: everything was easy to find and understand; I picked it up surprisingly quickly
• Vuze, in Java, huge number of tiny files strewn across multiple directories, hunting down which file did what was a mess, many files had similar names but none of them did very much, so it was hard to find where the "real logic" was, I did not understand why it was done that way or what the benefits were

Again, I'm mainly self-taught so I could be missing some big things, so if there are advantages I'm missing about the latter approach; I'd love to hear them.

[punmechanic]

I'm also self taught, but with respect: 5,000 lines is not "moderately sized" :smile:

The main benefit is just making things easier to read, it doesn't have to be a tiny file to see a benefit. Definitely a balance to strike between files that are too large and files that are too small, but when I to use the minimap in Sublime Text is when I start questioning if a file is too long.

The main reason to have a src directory is to split source files from things like scripts, package.json etc

[Zarel]

True! Which is why I agree with splitting up battle-engine! I'm just saying I don't understand the benefits of going full Java.

I think having a src directory might be worth it. Might.

[Morfent]

src and bin directories would be great to have for Golang, since it forces you to use those specific directories for source scripts to compile after you come to the point where you write with multiple modules.

Only problem is, I don't think an ELF is enough to give it's folder.

[Zarel]

Closing, since this was opened to ask a question and the question seems to be answered at this point.