Refactor remove_city to reduce complexity

[XHawk87]

Is your feature request related to a problem? Please describe.
267 lines long, this function carries out a large number of different tasks in sequence, and it's making it difficult to read and follow the logic, and writing a unit test would be all but impossible.

Describe the solution you'd like
Refactor so that this function orchestrates and all tasks are carried out in their own functions, each of which having a single responsibility.

Describe alternatives you've considered
It could be worth looking into a declarative style, looking at what needs to happen when a city is removed in terms of a set of rules rather than a sequence of steps. This could make it more composable, and allow better separation of concerns. For example, a module responsible for keeping the great wonder info up to date can be listening for city removal and various other events in order to do so.

Additional context

freeciv21/server/citytools.cpp

Line 1676 in df33c26

void remove_city(struct city *pcity)

[lmoureaux]

300 lines is long but it's not the worst function out there.

It could be worth looking into a declarative style, looking at what needs to happen when a city is removed in terms of a set of rules rather than a sequence of steps.

Pushing to the extreme, this could even be exposed to the ruleset (how do we rehome and disband units? do we lose part of our gold? do borders change? do we get some gold for remaining buildings?...).

For example, a module responsible for keeping the great wonder info up to date can be listening for city removal and various other events in order to do so.

This sounds like a great use case for the Qt signals/slots mechanism. We would need one QObject to emit the signal and another to receive it.

[XHawk87]

300 lines is long but it's not the worst function out there.

Well, that's true, but I don't think I should start out with attempting any big refactors. Something on the smaller end sounds about right to me.

Pushing to the extreme, this could even be exposed to the ruleset (how do we rehome and disband units? do we lose part of our gold? do borders change? do we get some gold for remaining buildings?...).

Yes! That would be fantastic.

This sounds like a great use case for the Qt signals/slots mechanism. We would need one QObject to emit the signal and another to receive it.

Very interesting. I guess it doesn't really matter if it was designed for the front-end, we can use it in server code too.

Should things like this be discussed at a higher level so we can follow a consistent approach across the project?

[lmoureaux]

Very interesting. I guess it doesn't really matter if it was designed for the front-end, we can use it in server code too.

Actually, the server already uses it for its networking and event loop.

Should things like this be discussed at a higher level so we can follow a consistent approach across the project?

Definitely.