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

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

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.

It might make sense logically to separate the common code up into 
several functions.  get_turn_income() might be useful elsewhere, for 
instance.
--- 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)?

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.
jason