[Freeciv-Dev] Re: [PATCH] try to cleanup assess_danger (PR#1321)

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Petr Baudis <pasky@xxxxxxxxxxx> wrote:
> Dear diary, on Tue, Mar 12, 2002 at 11:33:34AM CET, I got a letter, where
> Raahul Kumar <raahul_da_man@xxxxxxxxx> told me, that...
> > 
> > --- Markus Linnala <maage@xxxxxxxxx> wrote:
> > > 
> > > Change variable names to what they mean.
> > > 
> > 
> > I have some problems with thse patches, and the sequel to this patch. I
> agree
> > with everything Petr already said on these patches, and believe the
> > maintainers should carefully contrast what Petr has done to this patch.
> 
> Just please don't get me wrong. I think this patch helps, and is step in the
> right direction. It doesn't clean up anything by far, and I'm going to clean
> this up even more in the future, but it's question if it's blocker for this
> patch - as I can't tell you "ok, I'm going to send you the patch for it next
> week, then we'll do some brainstorming in following days and I assure I'll be
> available for making changes". Especially as I hadn't yet time to finish the
> proccess_*_want() patch.
> 

OK. The name changes for the variables are fine. But like I said I really hate
the code block tagged [EVIL CODE]. The question for a maintainer is, do you
want to live with this disgusting evil code, or should you demand replacement
with the nice cleaned up combat calcs *hint hint*. The way Markus goes in the
second patch is the wrong way in IMO.

<snip>

> > I like the name change from danger[1,2,3,4]. This seems to already be in
> Petr's
> > CVS though.
> 
> See above.
> 
> > >    int badmojo = 0;
> > 
> > No explanation of what this is.
> 
> Probably out of scope of this patch. It doesn't aim to be complete cleanup,
> contrary to many mine patches. .oO(dainty ;))))
>

If he's renaming variables, he should explain what the non obvious ones mean.
 
> > [EVIL CODE]
> > 
> > >    unit_list_iterate(map_get_tile(pcity->x, pcity->y)->units, punit)
> > >      if (unit_flag(punit, F_PIKEMEN)) pikemen = TRUE;
> > 
> > > +    if (unit_flag(punit, F_DIPLOMAT)) def_against_diplomat = TRUE;
> > >    unit_list_iterate_end;
> > >  
> > >    players_iterate(aplayer) {
> > > @@ -286,7 +286,7 @@
> > >            igwall = unit_really_ignores_citywalls(funit);
> > >            if ((is_ground_unit(funit) && v != 0) ||
> > >                (is_ground_units_transport(funit))) {
> > > -            if (dist <= m * 3) urgency++;
> > > +            if (dist <= m * 3) pcity->ai.urgency++;
> > >              if (dist <= m) pcity->ai.grave_danger++;
> > 
> > This seems like a candidate for the combat calcs. 
> 
> Especially candidate for YOU and your SINGLE_MOVE, isn't it? >:)
>

Yes. I agree, there should be no more mysterious 3's embedded in the code.
 
> > >            v *= v;
> > >  
> > > -          if (!igwall) danger2 += v * m / dist;
> > > -          else if (is_sailing_unit(funit)) danger3 += v * m / dist;
> > > -          else if (is_air_unit(funit) && !unit_flag(funit, F_NUCLEAR))
> > > danger4 += v * m / dist;
> > > -          if (unit_flag(funit, F_MISSILE)) danger5 += v * m / MAX(m,
> dist);
> > > +          if (!igwall) danger_wall += v * m / dist;
> > > +          else if (is_sailing_unit(funit)) danger_coastal += v * m /
> dist;
> > > +          else if (is_air_unit(funit) && !unit_flag(funit, F_NUCLEAR))
> > > danger_sam += v * m / dist;
> > > +          if (unit_flag(funit, F_MISSILE)) danger_sdi += v * m / MAX(m,
> > > dist);
> > >            if (!unit_flag(funit, F_NUCLEAR)) { /* only SDI helps against
> > > NUCLEAR */
> > >              v = dangerfunct(v, m, dist);
> > >              danger += v;
> > 
> > [/EVIL CODE]
> > 
> > This entire block should be replaced by a calculation from the combat
> calcs.
> > I really do not like this. My reason for voting against this patch is based
> > mostly on this block of code. 
> 
> It's not cleaning up anything there specifically. The main question here is,
> if
> we want small patches walking into right direction, or big patches jumping to
> the target.. It's kinda philosophical question however.


Like I said above, I like the name change. It just seemed to me that you
already
had this functionality in the code you're going to send.



__________________________________________________
Do You Yahoo!?
Try FREE Yahoo! Mail - the world's greatest free email!
http://mail.yahoo.com/