[freeciv-ai] Re: ripped out active diplomats patch...

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

Per,

At last, here is the list of my complaints on aidiplomat.[ch]
They are rated * (nitpicking) to **** (bug).

1(*) In ai_diplomat_city, ctarget must be adjacent, right?  I guess 
nothing bad happens if it isn't, but maybe mention that it should be, in 
the comment.  Good for understanding that this function won't attempt to 
send the diplomat on goto.

2(**) In the same function:
  if (pplayer->ai.control) {
    gold_avail -= pplayer->ai.est_upkeep;
  }
is a bit misleading.  I think you should just define 
  int gold_avail = pplayer->economic.gold - 2 * pplayer->ai.est_upkeep;

3(*) Please remove second blank line from the comment of 
  find_city_to_diplomat
One is already one too much to my taste.

4(*) In the same function,
  if (!ctarget || (best_dist > move_cost)) {
    if (!has_embassy
    /*....*/
    }
  }
can merge into one "if".

5(**) In the same function, why you don't change for 
already_considered_for_diplomat field?  Or is it only to prevent building 
too many diplomats?

6(***) In ai_diplomat_defend, what is 
  if (unit_type_flag(utype, F_DIPLOMAT)) {
for??  Who else would use the function?  Note that Spies also have this 
flag.

7(***) In ai_diplomat_bribe_nearby you should probably use moves_left 
instead of move_rate (in case someone will pass a unit with non-full 
moves).

8(various) ai manage_diplomat:
(**) I think it could be rearranged for better flow:
- check if we want to attack at all
- check if we have a valid target which makes sense and acquire one if we 
  don't
- walk to the target
- if we reached it, diplomat it.

(**) 
    if (dist == 1 && punit->moves_left > 0) {
      /* Do our stuff */
can be removed altogether, it is duplication of the code in the last block

(**) 
      /* Prepare to do our stuff (ie raise taxes) */
What if we aren't inciting?

(****) 
  /* Check if existing target still makes sense */
  if (punit->ai.ai_role == AIUNIT_ATTACK
      || punit->ai.ai_role == AIUNIT_ATTACK) {
The second instance should be AIUNIT_DEFEND_HOME.  And I think the check 
should be made earlier.

(**)
    } else {
      UNIT_LOG(LOG_VERBOSE, punit, "could not find a job");
    }
Why not return here?  I am always for early "return", it makes the code 
easier to read IMO.

(*)
  /* GOTO (unless we want to stay) */
  if (punit->ai.ai_role != AIUNIT_NONE && ctarget
      && punit->moves_left > 0
I think in the current configuration you cannot reach this place without 
having move_points.  So maybe assert(moves_left > 0)?

(****) 
        /* Check if we can achieve something... */
        if (punit->moves_left <= 0) {
          return; /* ... no */
        }
        if (real_distance_to_target(punit) == 1) {
          ai_unit_new_role(punit, AIUNIT_NONE, -1, -1);
          ai_diplomat_city(punit, ctarget);
        }
What if the role was DEFEND_HOME?



On Thu, 7 Nov 2002, Per I. Mathisen wrote:

> Ok, Greg, here is the active ai diplomats patch ripped out from the
> massive ai patch. It isn't tested except for an autogame... but it sure is
> small outside the aidiplomat files :)
> 
>   - Per
>