[Freeciv-Dev] Re: Military amortize (PR#1196)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Fri, Jan 04, 2002 at 05:06:24PM CET, I got a letter, where
Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
> To Raahul: Renaming variables like "e" should be done in one big renaming
> sweep IMHO, by you, or Petr, or myself, or whoever.  I wanted to keep the
> patches small :)
I agree. I wouldn't probably before week or two, but now I do ;).

> To Ross: I thought about making amortize and then military_amortized use a
> lookup table.  It can (and should) be done in next patches.  In any case,
> amortize is potentially buggy: on one hand it uses predefined MORT which can
> be changed elsewhere.  On the other hand it implicitly assumes that MORT ==
> 24 (in (23/24)^12 == 3/5 speedup).
And it explicitly notes it ;). We need comment for MORT with explicitly noted
it there as well.

> To Petr: On formatting: when I view my file everything looks perfectly
> formatted.  Can it be that diff chews the tabs up somehow?
Well, basically the tab jumps to the next virtual tabulation mark, which is
same column if you have "+ " there or not - but row of spaces has fixed length,
so it will end in different position when "+ " will be added. First it is
small, but as you start replying to the patch, it looks more and more funny ;).

> Sorry Raahul, I think the crazy formatting in your diff was also because of
> this -- I didn't know that :( On "MAX(1,c) in the function itself": here I
> had the same reason to leave it outside as suggested by Chris --- I want to
> leave freedom of passing c=0.
Ok.

> > > +          needferry = ((!sanity && !boatid && is_ground_unit(punit))
> > ? 
> > > +                40 : 0);
> > this deserves an if
> 
> I like ternary operators :(
> An if will take 5 lines :(
         needferry = ((!sanity && !boatid && is_ground_unit(punit)) ? 
                      40 : 0);
vs
         if (!sanity && !boatid && is_ground_unit(punit)
           needferry = 40;
         else
           needferry = 0;

I consider the second one much better readable - I like ternary operators only
when they actually simplify the code and the condition is not tooooo looong.

> > MORT);
> > > -          } else e = 0;
> > > +   e = military_amortize(b0, MAX(1, c), fprime);
> > this indendation looks odd - just tab-hallucination?
> 
> I guess... :(
> 
> A slightly improved patch is attached.  All seemingly unformated lines are
> due to tabulation thing -- do I have to hunt them out?
I'm not friend of mixing tabs and spaces. Either use one or the other, but
don't mix them.

> @@ -1314,35 +1334,31 @@
>                        g * SHIELD_WEIGHTING / (acity->ai.a * acity->ai.a + g 
> * d);
>            }
>            b0 -= c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
> SHIELD_WEIGHTING);
> -          if (b0 > 0) {
> -            a0 = amortize(b0, MAX(1, c));
> -            if (!sanity && !boatid && is_ground_unit(punit))
> -              e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + 40) * 
> MORT);
> -            else e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / (fprime * MORT);
> - /* BEGIN STEAM-ENGINES-ARE-OUR-FRIENDS KLUGE */
> -          } else if (!punit->id && !best) {
> -            b0 = b * SHIELD_WEIGHTING;
> -            a0 = amortize(b0, MAX(1, c));
> -            if (!sanity && !boatid && is_ground_unit(punit))
> -              e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + 40) * 
> MORT);
> -            else e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / (fprime * MORT);
> -            if (e > bk) {
> -              if (punit->id && is_ground_unit(punit) &&
> -                       !unit_flag(punit, F_MARINES) &&
> -                       map_get_continent(acity->x, acity->y) != con) {
> +       /* FIXME: build_cost of ferry */
<tab>
> +          needferry = ((!sanity && !boatid && is_ground_unit(punit)) ? 
> +                    40 : 0);
<tab><tab> + ternary evil ;))
> +       e = military_amortize(b0, MAX(1, c), fprime + needferry);
<tab>
> +       /* BEGIN STEAM-ENGINES-ARE-OUR-FRIENDS KLUGE */
<tab>
> +          if (b0 <= 0 && !punit->id && !best) {
> +            int bk_e = military_amortize(b * SHIELD_WEIGHTING, 
> +                                      MAX(1, c), fprime + needferry);
<taaaaaaaaaaaaaaaab>
> +            if (bk_e > bk) {
> +              if (punit->id && is_ground_unit(punit) 
> +                  && !unit_flag(punit, F_MARINES) 
> +                  && map_get_continent(acity->x, acity->y) != con) {
>                  if (find_beachhead(punit, acity->x, acity->y, &xx, &yy)) {
>                    *x = acity->x;
>                    *y = acity->y;
> -                  bk = e;
> +                  bk = bk_e;
>                  } /* else no beachhead */
>                } else {
>                  *x = acity->x;
>                  *y = acity->y;
> -                bk = e;
> +                bk = bk_e;
>                }
>              }
> -            e = 0; /* END STEAM-ENGINES KLUGE */
> -          } else e = 0;
> +       }
<tab>
> +       /* END STEAM-ENGINES KLUGE */
<tab>
>  
>         if (punit->id && ferryboat && is_ground_unit(punit)) {
>           freelog(LOG_DEBUG, "%s@(%d, %d) -> %s@(%d, %d) -> %s@(%d, %d)"

Otherwise the patch seems ok.

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
The advantage of GUI is that you can see everything you can change.
The disadvantage of GUI is that you can change only what you can see.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/