[Freeciv-Dev] Re: Economy report (PR#1307)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Mar 14, 2002 at 11:06:41AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> Raimar Falke wrote:
> > On Fri, Mar 08, 2002 at 04:26:08AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > wrote:
> > 
> >>Christian Knoke wrote:
> >>
> >>>CVS 08 MAR 2002
> >>>
> >>>The income in the economy report is irrational high.
> >>>
> >>>Savegame available.
> >>>
> >>That'd be a big oops...
> >>
> > 
> > Either this is fixed four times or only one time. The patch adds
> > client/repodlgs_common.[ch]. Tested with GTK and Xaw.
> > 
> > Andreas can you please test win?
> 
> Ahh, we think alike Raimar...
> 
> The current "bug" is that the taxable income is counted 29x too much. 
> This bug is in gtk, gtk-2.0 (obviously), and win32 (apparently; I had 
> not noticed this before) only.  If this patch is going to go in quickly, 
> that's fine.  But if it's not (say, if it's going to take a week or 
> more), I'd say apply the easy bugfix (just moving the city_list_iterate 
> outside of the impr_list_iterate; a 2-minute change with practically no 
> chance of breaking anything) now, and then worry about consolidation. 
> Current CVS is practically unplayable with this bug.
> 
> 
> That said, I agree: consolidation is a good idea here.  You've done it 
> almost exactly the way I would have, except you first build a full array 
> of improvement data and then iterate over it to build the dialog, 
> whereas I would have gotten one piece of improvement data at a time. 
> I.e. instead of
> 
>    struct improvement_entry entries[B_LAST];
>    get_economy_report_data(entries, &num_entries, ...);
>    for (i = 0; i < num_entries; i++)
>      add_improvement_to_report(entries[i]);
> 
> I would have done
> 
>    impr_type_iterate(impr_id) {
>      struct improvement_entry entry = get_economy_rpt_entry(impr_id);
>      if (entry.count > 0)
>        add_improvement_to_report(entries[i]);
>    } impr_type_iterate_end;
> 
> but this is almost entirely a matter of preference.  It would allow the 
> caller to show whichever improvements they want, pre-filtering or 
> sorting desired.  It would save a miniscule bit of (local) memory, at 
> the cost of an entirely insignificant amount of time.  It would intermix 
> the calculating and displaying code slightly.  So it's a trade-off.

In your interface either the caller has to add up all costs or an
extra function (which have to iterater of it again, no performance win
(but this isn't the point)). 

This function is custom-made for the users. What kind of service ;)

> It might make sense logically to separate the common code up into 
> several functions.  get_turn_income() might be useful elsewhere, for 
> instance.

I would rather merge/unify/whatever this with server/cityturn.c. But
it spread wide in server/cityturn.c. IMHO there are more important
things.

> --- OK, now on to practical issues.
> 
> In the new code, why use "j" as the iterator for the improvement?  As 
> long as we're adding all-new code, why not give it some sensible name 
> (id or impr_id)?

No problem on doing a s/j/impr_id/

> Um, and that's about it.  This looks like a solid consolidation of code. 
>   Aside from win32 and mui (which will have to be addressed by their 
> respective authors), I'd say this should go in pretty quickly.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "#!/usr/bin/perl -w
  if ( `date +%w` != 1 ) {
    die "This script only works on Mondays." ;
  }"
    -- from chkars.pl by Cornelius Krasel in de.comp.os.linux.misc