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

[Reinier Post <rp@xxxxxxxxxx>]

On Sun, Dec 09, 2001 at 01:19:04PM +0200, Juha Litola wrote:
> 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.

Yes, exactly.  I think your functions could do that better.

> 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. 

But it also marks them in the array, except the last one, which is
the return value.  This isn't clear from the name.
I'd prefer the name

  find_and_mark_unknown_prerequisites()

I'd also prefer it to just fill the array and return void.
That way it's much clearer what happens where.

> num_unknown_req_techs gives number of unknown requirement technologies
>   for given technology

Yes, by calling find...() and find...() does that itself.

The real problem I have here is that find...() is exported while num...()
isn't.  It should be the other way round.

Now when I look at the reason, it's because ai/aitech.c uses its own
marked array of requirement techs, the CACHE, which memorizes which
tech is a requirement for which, like my code did, except that it
happens every time ai_select_tech() is called.  This is inefficient.
And each time it will call your find...() function, which marks its own
array which makes it doubly inefficient.  It would really be better
to do this whole tech tree traversal once, hide it in your own code,
together with any caches you use to avoid recomputation, and
only expose a function to display results for use by ai/aitech.c.

> tech_cost gives cost of given technology
> 
> goal_tech_cost gives cost of reaching given goal

A goal tech is a tech, so why two functions?  I thin kthe names should
prevent this question.

> > 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.

[...]

The logic to compute the values is clear, but the updating of variables
I find confusing.

> > 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.

Yes, I got the wording wrong, sorry.

But req[tech] doesn't collect technologies, it numbers them, it would
be logical to update req[tech] with the return value before returning.

> > 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).

(See above.)

> > 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. 

I think you should move its 'CACHE' or some equivalent to your own code.
The behaviour won't be affected, but the interface won't be messy that way,
and the code will be more efficient as well.

> Also, doing it any other way would result in allocating
> memory in this function and needing caller then to free it.

I agree, that would be bad, but the code that uses it logically belongs
with your code in the first place ...

> > 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?

Perhaps.

> > 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.

Yes, it will be a little slower.

The most efficient approach simply stores all values (both req[] and
CACHE) for all players and only updates them when the tech tree is set
or a technology is researched.

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

-- 
Reinier