[freeciv-ai] Re: Review od Active Diplomats patch.

[Per I Mathisen <per@xxxxxxxxxxx>]

On Tue, 16 Apr 2002, Gregory Berkolaiko wrote:
> As for the code, it's of very high quality in my eyes.  Easy to read,
> quite well commented.

Thank you. I only mention some of the things you brought up. Most were
obvious things to fix.

[GB] I would change the above condition to:
if (!has_embassy(pplayer, city_owner(acity))
    || (city_owner(acity)->research.techs_researched
        < pplayer->research.techs_researched))
// If we haven't got an embassy with them, the embassy is at least as
// needed, as the technologies we will later steal.
Otherwise you won't get an embassy with a retarded civ: you'd exit
with negative want, see below.

Good catch.

[GB] I'll probably put myself in grave_danger of being crucified
by Petr if I say that I prefer && to be on the new line...

Agreed.

[GB] Don't we get some message by default from handle_diplomat_action?

Well, I want one here, since I just listen to debug messages from this
file when I want to debug diplomats...

+    /* Very unlikely but remotely possible */
+    freelog(LOG_NORMAL, "%s's diplomat %d could not find a target, "
+        "a city to defend or even a coastal city!", pplayer->name,
+        pdiplomat->id);

[GB] I guess you never saw it during testing ;)  But it should be LOG_DEBUG.
And what's this "coastal"?  Is it "closest"?

Never saw this. Very unlikely to happen. "coastal" because find_closest
will find us a coastal city, or give us NULL. No, I don't think this
should be LOG_DEBUG. The diplomat will be hanging around, lost in deep
thoughts (perhaps reading Machiavelli), so this is more like a bug.

[GB] What does assert(1) do?

Nothing. Bug.

+
+  /* Hackery, used by hostile AIs. This illustrated the desperate need for
+     a more intelligent build system - Per */
+  bool already_considered_for_diplomat;
 };

[GB] Don't be too critical, the above isn't too bad for a hack.
In fact I think think that this is _the_ right way.

Except I really dislike cluttering _other players'_ structs with our
information. Seems so... wrong.

But I'll moderate my comment ;-)

 bool player_has_embassy(struct player *pplayer, struct player *pplayer2)
 {
+  return (TEST_BIT(pplayer->embassy, pplayer2->player_no)
+          || (pplayer == pplayer2)
+          || (player_owns_active_wonder(pplayer, B_MARCO) &&
+              pplayer != pplayer2 && !is_barbarian(pplayer2)));
               ^^^^^^^^^^^^^^^^^^^
[GB] This is a tautology, no?

Good catch.

Yours,
Per

"Treason doth never prosper: what's the reason?
Why, if it prosper, none dare call it treason."
 -- Sir John Harrington (1561-1612)