Refactoring suggestions
Yann Dirson
What do others think ?
- color vs. player
We have a Color class, but it is used to represent players, and
sometimes the players are called "players", sometimes "colors". It
would probably be a good idea to call them "players" all around and
rename the Color class.
Further more, "black" and "white" are not suited to all games (notably
shogi, although there is a mapping convention to use them). So maybe
the 2 default colors should be renamed as well ? Not sure that would
be worth already, that could wait until we have a more complelling
reason.
- IType::canMove(), and "move type" in general
The name is badly chosen, since not only it answers to the "canMove"
question, it also adds information to the Move argument. At the very
least it should be renamed - maybe to something like
checkMoveAndFillMoveType() ?
OTOH, canMove() is not currently able to add the "drop" movetype
information - "drop" is not even a Move::type(), it is handled using
special Move methods (setDrop(), drop()) - wouldn't it be better to
make it fit into the "movetype" paradigm ?
Possibly by considering that the Move::src() for a drop is a valid
position [1]. Maybe making Point more generic, including a reference
to the "Board" it is on: pools would just be special kinds of "boards"
(rather a sibling to Board in the class hierarchy) [2]. That would
eg. allow to check state->valid(move) instead of
state->board()->valid(move.dst()) and such.
I am also concerned by the code duplication in the various
pseudolegal() implementations. Maybe once the drop gets handled as a
standard movetype, they would just end being identical except for the
movetype handling.
[1] (sidenote: having (Crazyhouse|Shogi)::Move::m_src be a
boost::variant<Point,Pool> would probably be a bad idea)
[2] and we would get at the same time some provisional support for
multi-board variants (Vulcan Chess and others from
http://en.wikipedia.org/wiki/Three-dimensional_chess).
- pseudolegal()
The intended difference between legal() and pseudolegal() is that the
former additionally checks for the "king in check" condition. Why do
we need such a specific method ? That difference is quite specific to
chess games (eg. it has no meaning for Othello) legal(), and it seems
to be the only called of pseudolegal().
One possibility would be to just merge it into legal(). But then,
given the above considerations about pseudolegal() current
reuseability, a better alternative could be to move the legal-specific
stuff into a better-named method, like forbiddenByKingCheck() (I
originally thought about isKingInCheck, but that's not so good in the
light of Sho-Shogi's multiple kings).
That, combined with the above suggestion, would bring us closer to
being able to specify only variant-specific stuff in legal() :
handling of specific movetypes, and call to forbiddenByKingCheck().
There would still be a lot of boilerplate code in legal() - the one
currently in pseudolegal(), asserting the consistency of the State
object. Wouldn't they be 1) better implemented as assertions that
would be ifdef'd out of production builds, 2) better enforced by State
itself ?
- split State
Some classes (at least Behaviour, Validator, Policy) have no data
member and virtually all their methods acceptt a State* as first
argument. In some way, they look like "aspects" of the State class -
and maybe we should call them just that (eg. moving them under a
StateAspect namespace), since they are otherwise not very structural,
and having them look like peers to State, Board and others does not
help to grasp the big picture.
Also, how do we decide which State aspect warrant to be in their own
component ? Ie, what does the current state of things allows us to
do, for which simply relying on Delegators::State is not sufficient ?
- namespaces
Since we have a core API usable by third-party plugins, we will want
to deploy core header files as part of the installation process. To
avoid stepping on other's toes, we probably need to put the core
API in a Tagua namespace.
However, the UI and variants stuff not being part of the public core
API, do we want them to be part of the same Tagua namespace ? Do we
want a TaguaPrivate namespace instead ? Do we just not use any
particular namespace for them, and possibly put the core in a
TaguaCore namespace instead, so it is still obvious what gets in it
and what does not ?
Best regards,
--
Yann
Yann Dirson
> - pseudolegal()
[...]
> One possibility would be to just merge it into legal(). But then,
> given the above considerations about pseudolegal() current
> reuseability, a better alternative could be to move the legal-specific
> stuff into a better-named method, like forbiddenByKingCheck() (I
> originally thought about isKingInCheck, but that's not so good in the
> light of Sho-Shogi's multiple kings).
Looking further into implementing this reveals a small difficulty: if
isKingInCheck() is not in IValidator, but introduced in
Chess::Validator, we would have to add a Delegators::ChessValidator
just to allow to derive other variants from it.
Now if the idea to merge Validator into State makes sense, we could
just add this function to DefaultState and be happy to already have
Delegators::DefaultState in place.
But then, DefaultState will finally be completely overloaded, and we
may want to look into other ways to complement standard interfaces and
implement default versions of them.
I have seen somewhere the idea of having Delegators autogenerated:
that would be really cool. Even cooler would be to get them
transparently used, so we don't need to take them (and m_delegator)
into account when writing methods for delegatable classes. That
reminds me of the OpenC++ project[1], which could have provided the
exact metaprogramming functionnality we'd need here (if only it was
not dead now, mostly because of its license issue and of it not
supporting templates at all). Maybe something like this could be done
with Synopsis[2] plugins, although it is surely quite some work
(esp. since the Synopsis parser currently has several problems with
the tagua code - I have reported the most important ones already).
But that could be an interesting sub-project in itself :)
Since I guess most of you do not know OpenC++, here is what the Tagua
code would be like if we apply a similar principle: there would be a
preprocessing step, which would basically write for us those
repetitive and ugly things (write Delegator classes, add m_delegator
members and insert them in the right places, etc.); we would just need
to add special keywords that would allow us to tag our classes and the
relevant members as delegatable. As show in [3] we could say
something like:
delegatable class IState {
delegated:
virtual void setup(...);
...
public:
// hm, none here :)
};
We would just have to write the "Meta-Object Protocol" (MOP) that
would allow the preprocessing phase to add create all the delegator
stuff by itself. I must say I am quite tempted to have a look at
doing that - if only I had more fre time :)
[1] http://opencxx.sourceforge.net/
[2] http://synopsis.fresco.org/
[3] http://opencxx.sourceforge.net/opencxx/html/base.html
Best regards,
--
Yann
Yann Dirson
> I have seen somewhere the idea of having Delegators autogenerated:
> that would be really cool.
Thoughts having matured somewhat... I am wondering where we should go.
Let's demonstrate my problem here - you can even have a look at it
with my attempt at reusing shogi promotion in minishogi [1]. It will
highlight the limitations of my above dreams - the sad part is that I
have not even found a way to solve the problem within the current
component-api framework. Hopefully one of you may have an idea...
While investigating the addition of promotions in shogi [2], I defined
Shogi::Behaviour::mayPromote(), which is indeed nearly generic enough
for most shogi variants, except for the hardcoding of promotion rows.
Now if I reuse Shogi::Behaviour as-is in minishogi, the promotion
rules are obviously wrong. So what about overriding mayPromote() ?
Well, for this I will need to use the Delegator pattern, and my
minishogi variant will be in need to link against a shared object
containing that delegator. Cmake disallows linking to a loadable
module (for god knows what reason); putting all such delegators in
taguacore does not sound quite right; and creating new extra libs in
the spirit of the defunct taguachesslib is not more appealing.
If I go the way of making mayPromote() generic, and having a
m_promotiondepth parameter, the problem is still there: setting that
parameter from minishogi will require being able to call a
Shogi::Behaviour method. It just makes things more complicated, by
needing to add a clone() mechanism like the one in State classes.
So are we stuck with the constraint that derived classes in variant
plugins may not add any new method, and that we would need to add them
to the core interfaces instead, and implement those variant-specific
methods as stubs in Default* classes ? That does not fit either in my
idea of genericity...
Where am I wrong ?
[1] http://repo.or.cz/w/tagua/yd.git?a=commitdiff;h=b253f1162b7ff2709394ca64b3fb3a0dd22e08c4
[2] http://repo.or.cz/w/tagua/yd.git?a=commitdiff;h=ca6d02e138efd0b1956c4e881be9d705d5b34a11
Yann Dirson
> Where am I wrong ?
Well, I may have found by myself ;)
I have probably just missed the point that delegators are just
implemented in .h files, and thus do not require link dependencies.
So I have started to play with the idea of implementing
Delegators::ShogiBehaviour - looks like the delegation support in
behaviours is not as completeas it is for states, so it may take some
time before I get something to work :)
Paolo Capriotti
On Mar 16, 9:33 pm, Yann Dirson <ydir...@altern.org> wrote:
> - color vs. player
This is a minor issue, I'll let you decide.
> - IType::canMove(), and "move type" in general
>
> The name is badly chosen, since not only it answers to the "canMove"
> question, it also adds information to the Move argument. At the very
> least it should be renamed - maybe to something like
> checkMoveAndFillMoveType() ?
> OTOH, canMove() is not currently able to add the "drop" movetype
> information - "drop" is not even a Move::type(), it is handled using
> special Move methods (setDrop(), drop()) - wouldn't it be better to
> make it fit into the "movetype" paradigm ?
>
> Possibly by considering that the Move::src() for a drop is a valid
> position [1]. Maybe making Point more generic, including a reference
> to the "Board" it is on: pools would just be special kinds of "boards"
> (rather a sibling to Board in the class hierarchy) [2]. That would
> eg. allow to check state->valid(move) instead of
> state->board()->valid(move.dst()) and such.
feasible. It introduces a lot of complications, and it's not something
we can implement that easily anyway. I'd rather try to address all
code duplication problems by introducing components. In this case, you
would have a IDropHandler component that is in charge of setting the
dropped piece of a move, and whose code can be shared across all
variants with drops.
> I am also concerned by the code duplication in the various
> pseudolegal() implementations. Maybe once the drop gets handled as a
> standard movetype, they would just end being identical except for the
> movetype handling.
checks that constraints on the dropped piece are satisfied, and shogi
checks similar (but different!) conditions.
> [1] (sidenote: having (Crazyhouse|Shogi)::Move::m_src be a
> boost::variant<Point,Pool> would probably be a bad idea)
> [2] and we would get at the same time some provisional support for
> multi-board variants (Vulcan Chess and others fromhttp://en.wikipedia.org/wiki/Three-dimensional_chess).
iteration.
> - pseudolegal()
>
> The intended difference between legal() and pseudolegal() is that the
> former additionally checks for the "king in check" condition. Why do
> we need such a specific method ? That difference is quite specific to
> chess games (eg. it has no meaning for Othello) legal(), and it seems
> to be the only called of pseudolegal().
(expecially chess variants) have similar notions, so IMO it may make
sense to keep it. Variants that do not have such notion can simply
neglect the difference and implement legal as a simple forwarding to
pseudolegal.
Having the pseudolegality check is useful for a number of things (and
it makes for a cleaner legality check code), so I vote to keep it.
> One possibility would be to just merge it into legal(). But then,
> given the above considerations about pseudolegal() current
> reuseability, a better alternative could be to move the legal-specific
> stuff into a better-named method, like forbiddenByKingCheck() (I
> originally thought about isKingInCheck, but that's not so good in the
> light of Sho-Shogi's multiple kings).
> That, combined with the above suggestion, would bring us closer to
> being able to specify only variant-specific stuff in legal() :
> handling of specific movetypes, and call to forbiddenByKingCheck().
> There would still be a lot of boilerplate code in legal() - the one
> currently in pseudolegal(), asserting the consistency of the State
> object. Wouldn't they be 1) better implemented as assertions that
> would be ifdef'd out of production builds, 2) better enforced by State
> itself ?
system. Having consistency checks is a good thing. And the boilerplate
code in legal is quite limited, I don't feel that as a big problem.
>
> - split State
>
> Some classes (at least Behaviour, Validator, Policy) have no data
> member and virtually all their methods acceptt a State* as first
> argument. In some way, they look like "aspects" of the State class -
> and maybe we should call them just that (eg. moving them under a
> StateAspect namespace), since they are otherwise not very structural,
> and having them look like peers to State, Board and others does not
> help to grasp the big picture.
>
> Also, how do we decide which State aspect warrant to be in their own
> component ? Ie, what does the current state of things allows us to
> do, for which simply relying on Delegators::State is not sufficient ?
splitting is reusability of components. Sometimes you only need to
change a detail in the state, and if this happens to lie in the
Behaviour interface, you only have to reimplement that one. I agree
that they are not that well defined (expecially Behaviour), so any
suggestion on how to reorganize this collection of classes is welcome.
>
> - namespaces
>
> Since we have a core API usable by third-party plugins, we will want
> to deploy core header files as part of the installation process. To
> avoid stepping on other's toes, we probably need to put the core
> API in a Tagua namespace.
>
> However, the UI and variants stuff not being part of the public core
> API, do we want them to be part of the same Tagua namespace ? Do we
> want a TaguaPrivate namespace instead ? Do we just not use any
> particular namespace for them, and possibly put the core in a
> TaguaCore namespace instead, so it is still obvious what gets in it
> and what does not ?
this one in a trac ticket.
Paolo