BOOL×BOOL→ENUM mapping

Question

Does this part look clean enough? Any suggestions on how to make it cleaner?

if (isCheck)
{
    if (isStuck)
    {
        return GameState.Mate;
    }
    else
    {
        return GameState.Check;
    }
}
else
{
    if (isStuck)
    {
        return GameState.Stalemate;
    }
    else
    {
        return GameState.Ok;
    }
}

Answer 1

I'd use a decision table here:

GameState[,] decisionTable = new GameState[2,2];
decisionTable[0,0] = GameState.Ok;
decisionTable[0,1] = GameState.Stalemate;
decisionTable[1,0] = GameState.Check;
decisionTable[1,1] = GameState.Mate;
return decisionTable[Convert.ToInt32(isCheck), Convert.ToInt32(isStuck)];

From Code Complete 2nd Edition, Chapter 19: General Control Issues, page 431:

Use decision tables to replace complicated conditions

Sometimes you have a complicated test involving several variables. It can be helpful to use a decision table to perform the test rather than using ifs or cases. A decision-table lookup is easier to code initially, having only a couple of lines of code and no tricky control structures. This minimization of complexity minimizes the opportunity for mistakes. If your data changes, you can change a decision table without changing the code; you only need to update the contents of the data structure.

Answer 2

I think ? operator will make your code more compact and readable:

return isCheck
    ? isStuck ? GameState.Mate : GameState.Check
    : isStuck ? GameState.Stalemate : GameState.Ok

Answer 3

As an alternative variant to @palacsint solution of decision table, the situation of a series of bool^n is traditionally represented using flags.

// in a global constants file somewhere
const Stuck = 1 << 0; // 1
const Check = 1 << 1; // 2
const ..... = 1 << n; // 2^n

int decisionTable[] = {
    GameState.Ok,         // 0b00
    GameState.Stalemate,  // 0b01
    GameState.Check,      // 0b10
    GameState.Mate,       // 0b11
};


// in code
int flags = 0;
if (isStuck(board)) flags |= Stuck;
if (isCheck(board)) flags |= Check;

return decisionTable[flags];

There is also an alternative way to construct the decision table that may be more readable and less error prone, though perhaps more verbose:

int decisionTable[] = new int[4];
decisionTable[0] = GameState.Ok;
decisionTable[Stuck] = GameState.StaleMate;
decisionTable[Check] = GameState.Check;
decisionTable[Stuck | Check] = GameState.Mate;

The issue with the decision table used by @palacsint or with writing it as functions with a series of argument (e.g. getState(isCheck, isStuck)) is that you have to remember the precise order of the arguments, which gets unwieldy when you have a large number of flags. For example, if you accidentally swapped the indices like: decisionTable[(int)isStuck][(int)isCheck]; you may find yourself into a lengthy debugging session without ever realizing the swap. Bitwise OR is commutative so it doesn't have this issue.

Answer 4

First of all, I think your solution is fine.

That being said, I can understand why the solution is frustrating. As the many other, varied answers suggest, this is one of those problems that just feels like it should have a simple, elegant, one-liner style solution. But, it just doesn't. There are shorter solutions, but they all sacrifice readability.

Like I said above, I think your approach is good, and if I came across it in code I was reviewing at work, I wouldn't bother to comment on it. But I do think it could be cleaned up just a little. When approaching a nested if I think it's best to start with the broadest question/condition in the outer if, then ask the more narrow question(s) below. Also, I think it's usually better to put the most common case near the top, because when someone is reading the code (perhaps trying to debug an issue), it's this branch that they're most likely to be interested in...also it just seems more logical to me. Finally don't be afraid to drop in a well named variable just to make the code more readable.

With all that in mind, here's my solution:

        var gameShouldContinue = ! isStuck;
        if (gameShouldContinue) {
            return isCheck ? GameState.Check : GameState.Ok;
        } else {
            return isCheck ? GameState.Mate : GameState.Stalemate;
        }

Answer 5

It depends on what you consider more readable, but you could use a binary-like way of expressing the variables as in a truth table:

isCheck isStuck   Value
=======================
T       T         Mate
T       F         Check
F       T         Stalemate
F       F         Ok

Which would translate to something more compact:

if (isCheck  && isStuck)    return GameState.Mate;
if (isCheck  && !isStuck)   return GameState.Check;
if (!isCheck && isStuck)    return GameState.Stalemate;
if (!isCheck && !isStuck)   return GameState.OK;

Answer 6

Any time you have multiple independent boolean variables representing a state, consider using a bitfield through the use of an enum with the FlagsAttribute applied. That way you don't need to maintain multiple variables to represent a single state.

[Flags]
public enum BoardState
{
    None = 0x0,
    IsCheck = 0x1,
    IsStuck = 0x2,
}

Then you could map these board states to the appropriate game state. Just make sure you map out every valid combination if you plan on using a dictionary.

private Dictionary<BoardState, GameState> transition = new Dictionary<BoardState, GameState>
{
    { BoardState.None,                         GameState.Ok },
    { BoardState.IsCheck,                      GameState.Check },
    { BoardState.IsStuck,                      GameState.Stalemate },
    { BoardState.IsCheck | BoardState.IsStuck, GameState.Mate },
};
public GameState Next(BoardState boardState)
{
    GameState nextState;
    if (!transition.TryGetValue(boardState, out nextState))
        throw new ArgumentOutOfRangeException("boardState");
    return nextState;
}

Otherwise use a switch statement if the mapping gets too unruly or you want to map multiple combinations to a single value (it's not necessary in this case).

public GameState Next(BoardState boardState)
{
    switch (boardState)
    {
    case BoardState.None:
        return GameState.Ok;
    case BoardState.IsCheck:
        return GameState.Check;
    case BoardState.IsStuck:
        return GameState.Stalemate;
    case BoardState.IsCheck | BoardState.IsStuck:
        return GameState.Mate;
    default:
        throw new ArgumentOutOfRangeException("boardState");
    }
}

Then use regular bit manipulation to set/clear the flags. Do take care in using this if you have values that represent multiple values

BoardState boardState = ...;
// to set a flag
boardState |= BoardState.IsCheck;
// to clear a flag
boardState &= ~BoardState.IsStuck;
// to test a flag
boardState.HasFlag(BoardState.IsCheck);

You can clean keep this nice and clean exposing this through boolean properties.

private BoardState internalBoardState;
public bool IsCheck
{
    get { return internalBoardState.HasFlag(BoardState.IsCheck); }
    set
    {
        if (value)
            internalBoardState |= BoardState.IsCheck;
        else
            internalBoardState &= ~BoardState.IsCheck;
    }
}
public bool IsStuck
{
    get { return internalBoardState.HasFlag(BoardState.IsStuck); }
    set
    {
        if (value)
            internalBoardState |= BoardState.IsStuck;
        else
            internalBoardState &= ~BoardState.IsStuck;
    }
}

Answer 7

Just to add on to the answer by @palacsint, if the language you are working with allows array literals, you can make it look like more a table:

var decisionTable = new GameState[][] {
  new GameState[] { GameState.Checkmate, GameState.Check } // isCheck
  new GameState[] { GameState.Stalemate, GameState.Ok    }
                      // isStuck
}
return decisionTable[(int)isCheck][(int)isStuck];

Answer 8

inline if statement is shorter but I don't know about cleaner

return isCheck
       ? (isStuck ? GameState.Mate : GameState.Check)
       : (isStuck ? GameState.Stalemate : GameState.Ok);

And to expand and simply the bitwise

int result = (isCheck ? 1 : 0) + (isStuck ? 2 : 0);
return (GameState) result;

enum GameState
{
    Ok = 0,
    Check = 1,
    Stalemate = 2,
    Mate = 3,
}

You can use the enum "as" your decision tree!

I arranged them

Answer 9

I think decision trees are unnecessary and over-complicated for a case this simple - although they have clear advantages if it gets more complicated than this, personally I'd do this:

if (isCheck)
    return isStuck ? GameState.Mate : GameState.Check;
else
    return isStuck ? GameState.StaleMate : GameState.Ok;

Which I find clearer than a pure ternary operator solution and more compact and thus more directly readable than a pure if solution.

Answer 10

If you like the idea of decision tables (like in palacsint's answer), then you could use more elegant approach using ValueTuple and Dictionary combo.

GameState GetGameState(bool isCheck, bool isStuck)
{
    var decisionDictionary = new Dictionary<(bool, bool), GameState>
    {
        [(true, true)] = GameState.Mate,
        [(true, false)] = GameState.Check,
        [(false, true)] = GameState.Stalemate,
        [(false, false)] = GameState.Ok
    };

    return decisionDictionary[(isCheck, isStuck)];
}

Answer 11

You could simply make GameState a Flags enum, which is merely a more compact version of the decision table palacsint proposed.

[Flags]
public enum GameState
{
    None = 0x0,
    IsCheck = 0x1,
    IsStuck = 0x2,

    Ok = None,
    Check = IsCheck,
    Stalemate = IsStuck,
    Checkmate = IsCheck | IsStuck,
}

When using the enum, you can set your two flags with bitwise or operators:

GameState current = GameState.Ok;
current |= GameState.IsStuck;
current |= GameState.IsCheck;

If you want to check the two bit flags on it, you can either use bitwise operators as follows:

var isStuck = ((current & GameState.IsStuck) == GameState.IsStuck);

or, in more recent versions of .NET, you can use the Enum.HasFlag method:

var isStuck = current.HasFlag (GameState.IsStuck);

The end result is that all your reading/writing of the flags is done entirely with your variable storing GameState, rather than having separate booleans floating around and a muti-dimensional array to look things up.

Answer 12

I think decision tables are good enough. If you want to go esoteric you can also define enum values as bit values and mapping would be automatic (which is very specific to the given example, so not a good idea in general):

enum GameState
{
  Ok = 0,
  Stalemate = 1,
  Check = 2
  Mate = 3
}

the code becomes:

return (GameState)(((int)isCheck << 1)|(int)isStuck);

Answer 13

So many crazy answers! I'll add another one! :D

The most important property of good code is no code duplication. In the original solution it is if (isStuck) condition, which is duplicated. We can avoid code duplication by making a clever (not so much in this case) abstraction!

T if_<T>(bool b, Tuple<T,T> t) {
  return b ? t.Item1 : t.Item2;
}
GameState choose (bool isCheck, bool isStuck) {
  return if_(isStuck, if_(isCheck, 
    Tuple.Create(
      Tuple.Create(GameState.Mate, GameState.Check),
      Tuple.Create(GameState.Stalemate, GameState.Ok)
    )));
}

Now the if_(isStuck,... is no longer duplicated. Win!

Answer 14

With C#8 switch expressions, you could do something like

private static GameState GetState(bool isCheck, bool isStuck)
    => (isCheck, isStuck) switch
    {
        (false, false) => GameState.Ok,
        (false, true) => GameState.Stalemate,
        (true, false) => GameState.Check,
        (true, true) => GameState.Mate
    };

Answer 15

Personally, I'd just collapse the tree.

if (isCheck && isStuck)
{
    return GameState.Mate;
}
else if (isCheck)
{
    return GameState.Check;
}
else if (isStuck)
{
    return GameState.Stalemate;
}
else
{
    return GameState.Ok;
}

or even

if (isCheck && isStuck) return GameState.Mate;
else if (isCheck) return GameState.Check;
else if (isStuck) return GameState.Stalemate;
else return GameState.Ok;

I find this much more readable than trying to do conversions to an Enum, and more straightforward than a decision grid. That being said, it doesn't scale as well as either option.

Answer 16

Not really recommending this, but you could use the fact that you are always using both variables to generate a string representaion of the four states, and then have either a dictionary or switch statement return the correct value.

string checkStuck = isCheck.ToString() + isStuck.ToString();
switch(checkStuck) {
   case "TrueFalse":
        return GameState.Mate;
   case "TrueTrue":
       return GameState.Check;
   case "FalseFalse":
        return GameState.Ok;
   case "FalseTrue":
        return GameState.Stalemate;
}

You could do the same with integers, but at that point you might as well go with Andy's answer.