<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40536 >
Alright, with the patch in 40563 this new goto code works
great, even better than the old warmap code.
All I could find is a minor display annoyance in that if
a fighter or bomber type unit has only 1 move left, then
the "turns to target" label shows 1 turn instead of 0 when
the user does a 1 tile goto. This is probably a bug in
the gui and can be left for another ticket.
Now we can discuss style/implementation improvements. :)
There are a few places with really long function names making
formatting kind of ugly. The style guide doesn't say anything
about this case, but I have been using this convention: to
have the extern/static and return type on a separate line
above the function name. So for example:
static enum tile_behavior no_fights_or_unknown_goto(
const struct tile *ptile,
enum known_type known,
const struct pf_parameter *p)
would become
static enum tile_behavior
no_fights_or_unknown_goto(const struct tile *ptile,
enum known_type known,
const struct pf_parameter *p)
I think this is a little better than having a line end in (
and the parameters floating in an arbitrary place. :)
I like the use of polymorphism for pf_map. Using enums
and switch statements is a nice way to implement this;
it is useful when there are a great number of very similar
"classes" and their overridden functions differ only
very slightly in their respective implementations (cf.
objprop in client/gui-gtk-2.0/editprop.c).
In this case I think a better design pattern would be
to have pf_map emulate a c++ abstract base class (or an
"interface" in the java terminology) and the 3 "derived"
pf_map classes use a vtable (a bunch of function pointers)
to emulate c++ virtual functions.
So the public interface for pf_map would look like
(obvious stuff left as "..." for brevity):
struct pf_map; /* Abstract base class. */
struct pf_map *pf_create_map(...); /* Factory function. */
/* Public function interface. */
void pf_destroy_map(struct pf_map *pfm);
struct pf_path *pf_get_path(struct pf_map *pfm, ...);
bool pf_get_position(struct pf_map *pfm, ...);
bool pf_next(struct pf_map *pfm);
void pf_next_get_position(const struct pf_map *pfm, ...);
struct pf_path *pf_next_get_path)(...);
struct pf_parameter *pf_get_parameter)(...);
(Maybe these should be renamed so that they all start
with "pf_map_", so that the object oriented style
is emphasized. Not strictly necessary for now; it can
be left as cleanup for later so that it does not clutter
up the patch needlessly.)
Internally (visible only to pf_map and derived classes)
we would have:
/* Abstract base class. */
struct pf_map {
void (*destroy)(...);
struct pf_path *(*get_path)(...);
bool (*get_position)(...);
bool (*next)(...);
void (*next_get_position)(...);
struct pf_path *(*next_get_path)(...);
struct pf_parameter *(*get_parameter)(...);
};
/* Down-cast macro. */
#define PF_MAP(p) ((struct pf_map *)(p))
void pf_destroy_map(struct pf_map *pfm)
{
pfm->destroy(pfm);
}
...
Or even struct pf_map could be made public so that
pf_destroy_map and friends could be inline functions
or macros (personally I would go for inline functions).
Now a derived class would look like:
struct pf_normal_map {
struct pf_map vtable; /* As before, must be first. */
... /* pf_normal_map specific stuff */
};
/* Up-cast macro. */
#define PF_NORMAL_MAP(p) ((struct pf_normal_map *)(p))
/* NB: The "constructor" returns a pf_map. */
struct pf_map *pf_normal_map_create(...)
{
struct pf_normal_map *pfm;
struct pf_map *vtable;
pfm = fc_calloc(...);
vtable = &pfm->vtable;
/* Initialize virtual function table. */
vtable->destroy = pf_normal_map_destroy;
...
return PF_MAP(pfm);
}
void pf_normal_map_destory(struct pf_map *p)
{
struct pf_normal_map *pfm = PF_NORMAL_MAP(p);
...
}
...
An ugly aspect of this kind of implementation is
the up-casting in every derived method, though it
is only 1 extra line per function.
Though not necessary for this relatively small
inheritance hierachy, type checking could be added
in the cast macros for example by keeping the
mode enum in struct pf_map (it wouldn't be a pure
"vtable" then, so some stuff would be renamed),
setting it correctly in each of the derived classes'
constructors, and checking it in the macro.
Since you already did the hardest part of the work,
if you don't feel like doing this kind of "cleanup",
I can do it myself later after committing this patch
(in a few days, to give more people time to look at
it and discuss).
-----------------------------------------------------------------------
そこでもう一度飛べて全世界のどこでも旅行ができる。
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
