search

We-the-People-civ4col-mod / Mod

This is the repository where the mod resides.
89 stars 37 forks source link

(Europe/Africa/Port Royal) contains messy and duplicated code #147

Open Nightinggale opened 5 years ago

Nightinggale commented 5 years ago

There are 3 trade screens. They are mostly identical, but they really lack code sharing. Instead it's copy pasted and then a few lines are changed. This means sometimes it's 3 sets of code, sometimes 2 sets and the function naming gives no indication of if the code is shared or not.

Even ignoring code sharing between the destinations, there is a lack of code sharing even for the same destination. AUTOMATE_SAIL_TO_PORT_ROYAL doesn't use canSailToPortRoyal, which leaves room for those two disagreeing on which units can actually go there.

Bug #24 was due to AUTOMATE_SAIL_TO_PORT_ROYAL copying AUTOMATE_SAIL_TO_EUROPE and instead of making the same function call, it copied the contents of the called function. It's to allow trading with Port Royal when Europe is locked, which seems ok. However it didn't copy all the checks meaning it resulted in false positive for when to show the automated button and suddenly we ended up with two buttons, all because of failure to reuse the code in a consistent and readable manner.

Ideally we don't have code specific for any destination. Instead we make a new xml file, which tells the setup for the trade screen in question. Perhaps we can replace a lot of the checks in "can this unit travel there" with an xml list of units, which are able to travel. This will certainly give modding freedom to future units instead of the current code where this question is answered by a combination of somewhat unrelated data from unitinfo.

Being allowed to go to a trade screen should be contained in a bool array in CvPlayer rather than various variables.

The "Europe screens" (python) could also be merged. They are mostly identical except for background images and other similar differences. This too could be placed in xml.

None of this is urgent as it seems to work right now. However it's something to keep in mind the next time we need to do something about this code because just patching it each time we have issues with it will not solve the underlying design problems.

devolution79 commented 5 years ago

Yeah, I noticed this as well when implementing AI_Africa(). It would be nice to make the ports fully generic. I think that the python screens are the worst offenders since they consist of mostly copy-pasted code. Note that the AI will have to be made generic as well.

ShadesOT commented 5 years ago

Just want to drop this here https://github.com/We-the-People-civ4col-mod/Mod/issues/197#issuecomment-432459319

devolution79 commented 5 years ago

Planning on overhauling quite a bit of the ship AI, so I will tackle the refactoring of the AI_sailTo* functions. I will ignore the python screens for now.

Nightinggale commented 4 years ago

Added the 2.8 milestone because I would like a future proof savegame format for the "Europe" screens. If it breaks savegames, then we will tend to not touch the code and since 2.8 breaks savegames anyway, we should include all savegame breaking changes in one go.