[Freeciv-Dev] Re: Tech cost patch v14 (PR#1082)

[Juha Litola <slave@xxxxxxxxxxxx>]

On Sat, Dec 08, 2001 at 07:12:51PM +0100, Reinier Post wrote:

> The first reason is the naming.  The function names look quite
> interchangeable to me, I can't really see from its name which specific
> role a function plays.  It would also help to #define symbolic names
> for the tech_cost_style.
Why should function name tell what role does the function play.
Function name should just say what it does, so it can be used 
anywhere where we need that functionality. Also names currently
tell pretty exactly what they do. E.g. non-static functions:

find_unknown_req_techs searches all requirements for technology
  that are not yet researched. 

num_unknown_req_techs gives number of unknown requirement technologies
  for given technology

tech_cost gives cost of given technology

goal_tech_cost gives cost of reaching given goal

> The second reason is the use of variables.  All variables are evil,
> but as long as I have a very clear idea of the purpose of every variable,
> I can check whether it is updated and read correctly.  Your code makes
> this hard, especially the following fragment:
I don't think variables are evil and I really can't see how this
function can be so difficult to understand. I'll go through it
line by line.

> +int find_unknown_req_techs(struct player *pplayer, Tech_Type_id tech, int 
> *req)
> +  if (tech == A_NONE)
> +    return 0;
If we have no requirement, there is no new requirement.
> +
> +  if(req[tech]++ || get_invention(pplayer, tech) == TECH_KNOWN)
> +    return 0;
Mark this technology as requirement in requirement array. If this
was marked as requirement before in recursive search, assign no
extra requirement. Also if technology is already known, it shouldn't
be added as requirement.
> +
> +  return 1 + find_unknown_req_techs(pplayer, advances[tech].req[0], req)
> +    + find_unknown_req_techs(pplayer, advances[tech].req[1], req);
Result of function is 1 + recursive search on requirement technologies.
> +}
> 
> The req[tech] value is updated, and causes recursive updates to the rest
> of the req array.  Both req[tech] and the function result appear to
> contain something like the number of required techs for 'tech', but they
> are not the same.  This is confusing.
req[tech] value is updated, but really doesn't "cause recursive
updates". req[tech] collects all technologies that are parents
for given technology. Function result is number of those technologies.

> You also rely on this function not being called with the wrong req array,
> they must be kept seprate for different players.
No, they need to be separate for different calls to this function (unless
you are doing some what-if thinking).

> The code
> 
>   if (req[tech]++)
> 
> seems wrong: surely you want to increment iff req[tech] was still 0,
> but instead you increment only when it was > 0 already.
Check your c-book. Expression is evaluated first, then compared
for truth value. if req[tech] is 0, postfix ++ operator will increment
it's value and then return the old value, which in this case is 0.
 
> The whole idea to make this function update an array without clearing it
> first is highly suspicious: if you call it twice with the same arguments,
> it will produce different results!  And this function is *not* static,
> so you have no control over where or when it is called.  This just can't
> be a good way to deal with the problem.
AI uses function in this style, I didn't want to alter its behaviour. 
Also, doing it any other way would result in allocating
memory in this function and needing caller then to free it.

> +static int num_unknown_req_techs(struct player *pplayer, Tech_Type_id tech) {
> +static int num_req_techs_rec(Tech_Type_id tech, int *counted) {

> Why don't you export one of these?
Because there is currently no use of them elsewhere. Also
num_req_techs_rec wont ever have use elsewhere as it is only needed
in precalc_tech_data. After that results can readily be read from
advances[tech].num_reqs.

> It would be better to have code in which the variables have a clear meaning.
> It should also be clearer which computations are player-specific and which
> aren't.
It is player specific if you pass struct player to it. Shouldn't
that be enough?

> One way to achieve this is to store the incidence matrix of the transitive
> closure of the tech tree, computing it once after the tech tree has been
> read, and to do all the rest without any auxiliary variables.
Yes, this is the other way of doing it. It is effectively the same,
but might be slower as all searches need to go through all technologies.
Recursive solution need only to go through necessary technologies.

> BTW can get_invention() be renamed to has_invented()?
I have no objections.

-- 
// Juha Litola

pgpU1Ztp6IXv5.pgp
Description: PGP signature