search

immortalityidle / immortalityidle.github.io

Immortality Idle Game Repo
32 stars 31 forks source link

Common state service #180

Closed LiquidAlmond closed 1 year ago

LiquidAlmond commented 1 year ago

I propose making all state within the app stored within a single object and operated on exclusively by a single service like this one. By making relatively small, pure functions to be passed into the apply and select function, the code can be more easily unit tested, which will make the code more robust. This is similar to how Redux and Recoil work, but a bit more primitive.

As a side effect, all services would be reduced to a set of independent functions callable from anywhere, aside from MainLoopService, which will still need to handle ticks.

immortalityidle commented 1 year ago

I'm afraid I don't understand how this makes anything better. This just seems like a lot of extra work without any significant gains. Why should the state relevant to a service not belong to that service?

LiquidAlmond commented 1 year ago

I'm afraid I don't understand how this makes anything better. This just seems like a lot of extra work without any significant gains. Why should the state relevant to a service not belong to that service?

LiquidAlmond commented 1 year ago

Well... didn't intend to do that.

If the state was actually relevant to the single service, then you would be correct, but various bits of state are used by multiple services. For any variables accessed (read or updated) outside of a single service, it's better for it to be places in a central location to minimize circular dependencies and number of services injected.

For example, health is used by ActivityService, BattleService, CharacterService, Character (holder of the actual state), HellService, HomeService, ImpossibleTaskService, ItemRepoService and the health panel. Moving health to a StateService would remove the dependency of all these services on Character (for health at least) and gives more easily testable code.

immortalityidle commented 1 year ago

Hmmm, still not sure I'm seeing the benefit. Circular dependencies seem like they're not a problem to me, is there something dangerous or performance reducing about them I'm not aware of? And I wasn't planning to add unit testing to the game ever. In any case, what you're describing doesn't seem to be what this PR actually does. This just looks like it creates an extra set of state variables hanging out in their own service.

LiquidAlmond commented 1 year ago

Right. I don't intend for this exact PR to get merged in. I was demonstrating something that could be done to help simplify the code. Getting everything else in a state where this service would be useful would involve a lot of work and the meat of why would be lost in such a PR.

Want me to actually update a panel and a service so you can see what they would look like with this change?

LiquidAlmond commented 1 year ago

Canceling due to this being a much larger pain in the butt than I thought it would be with significantly less benefit.