Complete.Org: Mailing Lists: Archives: freeciv-dev: March 2002:
[Freeciv-Dev] Re: Economy report (PR#1307)

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] Re: Economy report (PR#1307)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Thu, 14 Mar 2002 11:06:41 -0800 (PST)

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++)

I would have done

  impr_type_iterate(impr_id) {
    struct improvement_entry entry = get_economy_rpt_entry(impr_id);
    if (entry.count > 0)
  } 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.


[Prev in Thread] Current Thread [Next in Thread]