[Freeciv-Dev] Re: (PR#1098)

[Mike Kaufman <mkaufman@xxxxxxxxxxxxxx>]

On Thu, Dec 06, 2001 at 10:34:29PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> Vasco Alexandre Da Silva Costa wrote:
> 
> > Seems good. However tile_is_known() returns an enum. The name sounds like
> > a predicate name.
> > So wouldn't returning (map_get_tile(x,y)->known>=TILE_KNOWN_FOGGED) make
> > more sense? 
> 
> tile_is_known is definitely used in inconsistent ways.  It is used in 
> roughly 40 places, of which about half use it as a boolean (which the 
> name would make you think it is).  But the others compare it...at a 
> glance I see
>    tile_is_known(...) >= TILE_KNOWN_FOGGED (equivalent to boolean)
>    tile_is_known(...) == TILE_KNOWN_FOGGED
>    tile_is_known(...) == TILE_KNOWN
>    tile_is_known(...) != TILE_UNKNOWN (equivalent to boolean)
> 
> So, I don't think changing it to a boolean is reasonable - but it could 
> be renamed tile_get_known or something to that effect, which would 
> remove the appeareance of it being a boolean.  Taking it one step 
> further, you might want to make all of the uses consistent with each 
> other, but this is probably overkill.
> 
> Server-side, the equivalent function is called map_get_known.  But then, 
> the server usually deals with "map positions" while the client deals 
> with "tiles", so I think tile_get_known is a reasonable name, assuming 
> you don't want to duplicate the server's name of map_get_known().
> 
> 
> Oh, one other thing that could use improvement: the boxed comment about 
> the tile_is_known function.  This should IMO describe the server/client 
> descrepancy of freeciv, and only mention civworld as a side note.
> 
> Attached is a variant patch where I've renamed tile_is_known to 
> tile_get_known.  I also changed the boxed comment a little (Mike: you 
> may want to fix it).
> 

patch looks fine, comment looks fine. naming looks fine. I'll assume it
compiles. Maintainers should apply this patch. Thanks for saving me the 
work of converting the names.

-mike

> jason