Follow-up Comment #19, patch #5306 (project freeciv):
>As you probably have noticed I'm not as quick to respond as I was earlier
this week. I think I will be able to take time for a new review tomorrow
afternoon. Don't worry if you don't have a new iteration by then. I'll
probably have time for a review again early next week or maybe even at the end
of the week end.
No problem Sveinung.
>I understand. Freeciv is a large code base. Even with tools that helps you
navigate it there is a lot of new stuff.
Exactly.
>Of do_expel_unit()?
Yes.
>Comment will need an update.
Done.
>Who is this a link to? (Please rename for clarity)
Done.
>Who is supposed to be on this tile? (Please rename for clarity)
Done.
>The code tries to find the city a diplomat will flee to. Pick the comment or
the code.
Done.
>You got the owner of the wrong unit. You want to find a city belonging to the
target so you can send it there. (Or just go for the capital in stead)
Done.
>Let us keep things simple. Abort if no city is found. (pcity == NULL means
that no city was found) Notify the actor player that there is no place to send
the target and insert a return statement. (You have written notice code below)
Done.
> Remove the capital test. If you decide to expel the target to his capital
you could add a fc_assert with is_capital() in stead. Since you aborted if
pcity wasn't found you don't have to test it.
Done.
> Not sure if I understand what you were trying to do here. Let me know if I
got you wrong.
I don't understand too. An old piece of code without sense... Anyway I've
deleted.
> This is the notice I mentioned above. English issue (I'm not a native
speaker): I think it should be "You didn't expel"
I know... Anyway "Done".
> Copy past error.
Done.
----
>I don't think that not finding a place to send someone should count as
getting caught/stopped. I'm open to counter arguments.
It's an interesting point.
> The expulsion didn't fail when your own unit was expelled. The unit didn't
die so it wasn't lost. While the expulsion was a success the chance of failure
wasn't all that high. The question is "you did this" vs "someone did this to
you", not loss or success.
I agree.
Expel Failed > Expel Suffered
Expel Succeeded > Expel Accomplished
>Was your goal to avoid introducing a third event type?
No, my goal was to write the code correctly and learn something. Now I see
other possibilities about "expel units".
Expel unit: accomplished (1) or not (2)
If (1) you expel the *unit* > Ok.
If (2) you can't expel the *unit* > Ok... Enslave? Imprison? Murder? Hostage?
*Who can we expel?* Settler (the most annoying unit in the game), workers,
military unit; if actor unit > target unit... i.e. *Pikemen > Warriors,
Phalanx* or *Knights > Horsemen, Chariot*.
What do you think?
(file #24704)
_______________________________________________________
Additional Item Attachment:
File name: expel_units.patch Size:16 KB
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?5306>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
