Henk wrote: ↑Fri Jul 31, 2020 1:20 pm
[...] nested ?: at the last statement of this method are hardly readable.
Code: Select all
(IMovePrioQ, IMoveBase, int, int) NextMove( SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
{
var end = i >= prioQ.Count();
var (j, nextStage) = end ? (0, stage+1) : (i, stage);
var nextPrioQ = (end && (nextStage == 2 || nextStage == 3)) ? new MovePrioQ(
(nextStage == 2 ? p.position.CollectCapturesPromotions(true): p.position.CollectNonCaptures()
).Select(m =>
(ComputePriority(PSQTableEval, allowStandPat, KillerMoves,
MoveRatings, m, p.position, p.depth), m)).ToImmutableList()).Sort()
: prioQ;
var mv = j < nextPrioQ.Count() ? nextPrioQ[j]: null;
return (mv == null
|| nextStage > 1 && !LegalMove(p.inCheck, mv, p.position)
|| nextStage == 3 && allowStandPat && !p.position.GivesCheck(mv)
|| nextStage > 3
)
? (nextStage > 3) ? (nextPrioQ, null, -1, -1): NextMove(p, nextPrioQ, nextStage, j + 1, allowStandPat)
: (nextPrioQ, mv, j, nextStage);
}
I could imagine the following might be slightly more readable, and maybe even a bit more accurate:
Code: Select all
IMovePrioQ CollectPseudoLegalMoves(SearchParam p, int stage, bool allowStandPat)
{
// ASSERT(stage == 2 || stage == 3);
return new MovePrioQ((stage == 2 ? p.position.CollectCapturesPromotions(true): p.position.CollectNonCaptures())
.Select(m => (ComputePriority(PSQTableEval, allowStandPat, KillerMoves, MoveRatings, m, p.position, p.depth), m))
.ToImmutableList()
).Sort();
}
(IMovePrioQ, IMoveBase, int, int) NextMove( SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
{
// ASSERT(stage >= 1 && stage <= 3);
// ASSERT(i >= 0);
if (stage > 3)
{
return (prioQ, null, -1, -1);
}
if (i >= prioQ.Count())
{
if (stage + 1 > 3)
{
return (prioQ, null, -1, -1);
}
return NextMove(p, CollectPseudoLegalMoves(p, stage + 1, allowStandPat), stage + 1, 0, allowStandPat);
}
var mv = prioQ[i];
if ((stage > 1 && !LegalMove(p.inCheck, mv, p.position))
|| (stage == 3 && allowStandPat && !p.position.GivesCheck(mv)))
{
return NextMove(p, prioQ, stage, i + 1, allowStandPat);
}
// NOTE:
// I suggest to switch the order of "i" and "stage" in the return tuple for consistency with the
// order of parameters of NextMove()
return (prioQ, mv, stage, i);
}
Some comments:
1) I moved the pure move generation part into a separate function CollectPseudoLegalMoves().
2) I tried to avoid ?: expressions as much as possible, and only used them when their meaning was immediately obvious.
3) I did not follow the "single point of return" philosophy since I favor the "early return in special case" philosophy which also saves a lot of indentation. One may of course insert some "else" or "else if" statements if that style is preferred.
4) I started by expanding all logical paths through the code into separate blocks, then removed all unreachable parts (due to conditions that were always false), then simplified again by avoiding variables that were used only once.
5) By doing the above, I think I also avoided a pathological case where a call of NextMove() with stage < 3, i >= prioQ.Count() and nextPrioQ.Count() == 0 led to a recursive call of NextMove() fed again with nextPrioQ and i=1.
6) I do not like the approach of replacing loops by recursion very much, I just left it in here in order to not change everything at once ...
. I just did it for fun to see if it was possible.
.