[Freeciv-Dev] Re: [Patch] Map coord. to city map transformation and back

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Sep 26, 2001 at 12:17:52AM -0400, Ross W. Wetmore wrote:
> These utility routines are truly messterpieces of obfuscation. Are
> you sure you couldn't have added a few more levels of function calls
> to transform the argument type and order a couple more times. I like
> cup and pea games.

It is good to get at least some comments.

> And you've only used half a line for the function names and nothing
> like that for the arguments. The fact is that you declared this
> function in only 3 lines of code when clearly you could have done much
> better than that. Why not "local_nearest_to_real_close_by_city_map_x" as
> a replacement variable name to keep the typing fingers warm in winter
> as well as making sure that people really get the message.

You can suggest shorter names.

> It is also the case that one can get rid of the O(n) iteration in 
> get_citymap_xy()

I noticed it. Either we encode special knowledge or we leave to loop.

> (sample below, was in corecleanup_07e), but perhaps you 
> would rather add some sort of loop to the other routines to balance 
> things out more elegantly.
> 
> All I can say is .... WOW (Where O Where ...) :-)
> 

> Actually, other than this stuff it was pretty good.

Thanks.

> /**************************************************************************
> ...
> **************************************************************************/
> int is_valid_city_coords(const int city_x, const int city_y)
> {
>   if ((city_x == 0 || city_x == CITY_MAP_SIZE-1)
>    && (city_y == 0 || city_y == CITY_MAP_SIZE-1))
>     return 0;
>   if ( !RANGE_CHECK_0(CITY_MAP_SIZE, city_x)
>     || !RANGE_CHECK_0(CITY_MAP_SIZE, city_y) )
>     return 0;
> 
>   return 1;
> }

Can you make a patch with RANGE_CHECK and RANGE_CHECK_0 and the
changes to is_valid_city_coords?

> /**************************************************************************
> Given a real map position and a city, finds the city map coordinates of the
> tiles.
> Returns TRUE (== 1) if the real map position was inside the city map.
> **************************************************************************/
> int get_citymap_xy(const struct city *pcity, const int x, const int y,
>            int *city_x, int *city_y)
> {
>   /* Get x,y relative to the NW corner of pcity */
>   *city_x= (x - pcity->x + CITY_MAP_SIZE/2);
>   *city_y= (y - pcity->y + CITY_MAP_SIZE/2);
> 
>   /* If coords can wrap, we can be off by +/- xsize,ysize. */
>   if( has_mapwrap(MAP_TYPE_WRAPX) )
>     *city_x= map_wrap_x(*city_x);

Your map_wrap_x method does really work with city coordinates
(0,...,4)?

>   These should be replaced by get_citymap_xy, just as map_adjust_* are
>   replaced by normalize_map_pos.

Full ack.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Reality? That's where the pizza delivery guy comes from!"