SpaceRace enum dehardcoding about open-realms-of-stars HOT 5 CLOSED

Spork bonus ship could be removed, since all those spaceraces require new balancing any way.
That image alignment and ship images could be just pointer to another object/class in SpaceRace class eventually.

from open-realms-of-stars.

There was at least one case where pseudo-race's ID in createOne() method was wrongly written. I suggest to create a static constant and use it instead of writing it each time. Also, using the Optional<> or createOr() methods can help to detect such cases, as it forces programmer to explicitly say, what to do in case that value is absent, possibly throwing an error when something goes awry. (I found it. It is in Diplomacy.java, text SpaceRaceFactory.createOne("SPACE_PIRATES")); It is possible there are other such cases.)

Good catch, I had these bunch of place with _. Using static would be much bettter.

Regarding design, if you plan to ever introduce "races created at runtime" (procedurally-generated or similar), the SpaceRaceFactory class usage and code will likely have to be revised dramatically. This is because of the (dreaded) global state. It is still very "enumish" and thus not suitable for such design - in case of RaceTraits, Buildings, "Bridge Graphics", etc. global state is not such a deal-breaker, as those objects are likely to be predefined before program launch (by programmer/modder) and work as available "blocks to build from". Races are more a thing that "can be built from the available blocks" at runtime (think like Planets vs. Buildings - Buildings are predefined in data files, their individual "setup" on a Planet is not).

My idea was to add space race editor directly into main menu. When player click load game or new game then SpaceRaces would reinitialized. Currently there is no way to do, but it should be just static method that calls singletons init() method.
In actual editor there should be warning if space race is deleted that old save files might not fuction anymore.

So technically, the SpaceRaceFactory now works as a "race preset loader". And by quick glance, a whole lot of code is still designed to treat SpaceRaces as "globally available information" rather than "ad-hoc data". Usage of the createOne() method of the factory can help to find such cases. Removing the global-state of SpaceRace data will also likely include things like saving race data to save files or something. But since SpaceRace is normal class with normal objects now, potential removal of the global state is at least conceivable. 😄

And if you do not plan to add "races made at runtime", and leave this to "modding" only, then everything's great!

That editor from main menu is a bit like tool for modding. Just easier UI than opening JSON editor.

In any case, the removal of the SpaceRace enum is still a major step forward. 👍

Yes, I agree.

from open-realms-of-stars.

@tuomount I suggest you look into the StarMap-related stuff. The file is hard to navigate and refactor for me (it is over 4500 lines of code, with many responsibilities).

Additionally, it might worth splitting StarMap.java, moving map generation elsewhere, so the file is more focused and understandable (and shorter!). I don't consider behavior of StarMap generation as something that StarMap class should do, since functions as "game board" object for runtime of a single game match.
A StarMapGenerator class or something is practically a must.

from open-realms-of-stars.

@tuomount I quickly reviewed the code in the little free time I have these days and I saw you have managed to successfully dehardcode the SpaceRace enum. That's marvelous! Congratulations! 🥳
Here is some feedback on the changes and outcomes, if you are interested.

There was at least one case where pseudo-race's ID in createOne() method was wrongly written. I suggest to create a static constant and use it instead of writing it each time. Also, using the Optional<> or createOr() methods can help to detect such cases, as it forces programmer to explicitly say, what to do in case that value is absent, possibly throwing an error when something goes awry.
(I found it. It is in Diplomacy.java, text SpaceRaceFactory.createOne("SPACE_PIRATES")); It is possible there are other such cases.)

Regarding design, if you plan to ever introduce "races created at runtime" (procedurally-generated or similar), the SpaceRaceFactory class usage and code will likely have to be revised dramatically.
This is because of the (dreaded) global state. It is still very "enumish" and thus not suitable for such design - in case of RaceTraits, Buildings, "Bridge Graphics", etc. global state is not such a deal-breaker, as those objects are likely to be predefined before program launch (by programmer/modder) and work as available "blocks to build from". Races are more a thing that "can be built from the available blocks" at runtime (think like Planets vs. Buildings - Buildings are predefined in data files, their individual "setup" on a Planet is not).

So technically, the SpaceRaceFactory now works as a "race preset loader". And by quick glance, a whole lot of code is still designed to treat SpaceRaces as "globally available information" rather than "ad-hoc data". Usage of the createOne() method of the factory can help to find such cases.
Removing the global-state of SpaceRace data will also likely include things like saving race data to save files or something.
But since SpaceRace is normal class with normal objects now, potential removal of the global state is at least conceivable. 😄

And if you do not plan to add "races made at runtime", and leave this to "modding" only, then everything's great!

In any case, the removal of the SpaceRace enum is still a major step forward. 👍

from open-realms-of-stars.

There are no more SpaceRace enum. Closing.

from open-realms-of-stars.