Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
12  
I think this is one of the cases where leaving out (some of) braces would improve your code. –  svick Feb 17 '13 at 13:56
2  
I don't think any of the answers are more readable than the original code. And it looks like it's a standalone method already, so elsewhere you'll just be calling GetGameState(isCheck, isStuck). –  James Feb 17 '13 at 20:33
3  
I do not think you need this enum. Ennum is an intermediate value, not a goal. With just IsStuck and IsCheck your code can: if (isStuck) { displayReason(); GameOver();} ... else ... if (IsCheck) { SayCheck(); }. If you generate the enum, then you still need to run a bunch of conditions on it. –  Leonid Feb 18 '13 at 5:01
2  
@James It fills half the screen, and it can cleanly be written in three lines (nested ternary). I don’t understand why there’s even a discussion about the relative benefits of these solutions. –  Konrad Rudolph Feb 18 '13 at 8:19
add comment

15 Answers 15

up vote 48 down vote accepted

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.

share|improve this answer
7  
Just remember to isolate this in a function. –  luiscubal Feb 17 '13 at 18:43
6  
I think with just two cases like this, a decision tree is actually harder to follow. A more compact presentation of the OPs code would be cleaner and easier to understand. –  Jack Aidley Feb 18 '13 at 10:40
2  
palacsint, I took the liberty to turn your pseudocode into real, compiling C#. If you have any objections, feel free to rollback my edit. –  codesparkle Feb 21 '13 at 20:25
    
@codesparkle: I'm glad that somebody did it :) Thank you! –  palacsint Feb 21 '13 at 21:39
add comment

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

return isCheck
    ? isStuck ? GameState.Mate : GameState.Check
    : isStuck ? GameState.Stalemate : GameState.Ok
share|improve this answer
11  
THis one isn't too bad; but I find ternary operators become hard to follow when nested much faster than other operators and try very hard to avoid doing so. –  Dan Neely Feb 17 '13 at 17:18
1  
This is how I’d always write it. –  Konrad Rudolph Feb 18 '13 at 8:18
3  
I'd bracket the code for clarity but I think this is perfectly acceptable. –  Jack Aidley Feb 18 '13 at 10:41
3  
@DanNeely Agree with you, but 1-level nesting seems reasonable in this case. Decision table as for me is an overkill here, especially given that you need to map bool->int first, or use Dictionary to map booleans directly –  almaz Feb 18 '13 at 10:51
add comment

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.

share|improve this answer
7  
honestly looking at the OP's code I can understand that better than this code. Bit shifting has its place in lots of different contexts, but I don't see this one being beneficial for this purpose. –  Robert Snyder Feb 17 '13 at 17:55
    
@RobertSnyder: you only need to write those global constants once, but you used the flag and the flag tables much more often. An alternative to bit shifting would be to use power(2, n) but IMO bit shifting translates intent more clearly than pow() because flags are not really integers/numbers but a series of booleans (a series of bits). –  Lie Ryan Feb 17 '13 at 23:54
2  
More recent versions of C# support named parameters, so you can write something like getGameState(isStuck: this.isStuck, isCheck: this.isCheck). Then the exact parameter order does not matter. –  Michael Kjörling Feb 18 '13 at 10:18
add comment

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;
        }
share|improve this answer
    
“ There are shorter solutions, but they all sacrifice readability.” – wrong. The nested ternary solution doesn’t. It’s idiomatic in about any language and directly communicates what it does. –  Konrad Rudolph Feb 18 '13 at 17:05
2  
Eh... In this case nested conditional operators wouldn't be too terribly hard to read if it's well formatted, but as a general rule of thumb, I think it's bad practice. Not because it can't be understood, but because it takes more time to understand and offers very little benefit. YMMV –  Andy Feb 18 '13 at 22:12
    
We’re talking about the specific case of one level of nesting here though. There isn’t any problem with that – and in general chaining the ternary operator is never hard to read if properly formatted (except in PHP because the language sucks and gets the precedence wrong) – in fact, it reads just like a switch statement. Here’s an example in C++. Again, this is idiomatic code in most languages. –  Konrad Rudolph Feb 19 '13 at 11:54
1  
@KonradRudolph I for one thing this is much easier to read than a nested conditional operators (at least with C#'s ugly ?: syntax.) –  CodesInChaos Feb 19 '13 at 15:25
    
@CodesInChaos Did you take a look at the linked code? If you have trouble reading this then this suggests lack of familiarity with the language rather than lack of readability of the idiom. Sorry, but that’s just it. Incidentally, I agree that the ?: operator in the C-family languages is really ugly, and that there is better syntax for this in other languages. –  Konrad Rudolph Feb 19 '13 at 15:42
show 2 more comments

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;
share|improve this answer
8  
This is actually much more complex and error-prone than the original code. Here is why: four ifs instead of three; four 2-literal boolean expressions instead of three 1-literal ones; mutual exclusion not structurally obvious. –  Rotsor Feb 17 '13 at 17:32
2  
I can agree on the complexity based on those metrics. However, take the example of design patterns: can you say they introduce complexity in the form of number of classes, for example? Sure they do. That's why I pointed out where the design decision comes from: a truth table. Just as you could be baffled by looking at the many classes used for,say, Java streams, when you figure out they follow the decorator pattern everything seems more clear all of a sudden. I proposed that answer since I've wound up with a need for that kind of functions where clarity and compacity are the most valued. –  Javier Villa Feb 18 '13 at 0:12
3  
Since return actually returns, you can make this slightly less verbose by skipping all the else keywords :) –  Svish Feb 18 '13 at 10:12
1  
@JavierVilla: you can remove the isCheck from test 3 and turn test 4 into a simple return. That makes it no more complex than the original. –  jmoreno Feb 18 '13 at 18:44
1  
One problem with this is that you need to add something below the four ifs, since the compiler doesn't understand that they handle every possible case. –  CodesInChaos Feb 19 '13 at 15:27
show 2 more comments

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];
share|improve this answer
    
I think you could even simplify this more if you consider that his arguments are a rudimentary binary counting exercise. That being said if you could change isCheck and isStuck to a simple flag enum, you could just OR the numbers and return the appropriate GameState. –  Robert Snyder Feb 17 '13 at 18:00
    
Don't forget that original @palacsint answer is a pseudocode, you can't cast bool to int –  almaz Feb 18 '13 at 10:55
add comment

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

share|improve this answer
add comment

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;
    }
}
share|improve this answer
add comment

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.

share|improve this answer
add comment

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);
share|improve this answer
    
Why would he want to "go esoteric"? –  Benjamin Gruenbaum Feb 18 '13 at 18:21
    
He wouldn't probably. But knowing all alternatives might expand our horizon when looking for solutions in similar problems in the future. And that could be a perfect solution for different set of constraints. –  ssg Feb 18 '13 at 21:15
add comment

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!

share|improve this answer
add comment

No need to talk much I think,

             stuck
          |--------- mate (11)
   check  |
|---------|
|         |
|         |--------- check (10)
|          
|            stuck
|         |--------- stalemate (01)
|         |
|---------|
          |
          |--------- ok (00)

enum GameState
{
  Ok = 0, // 0x00
  Stalemate = 1, // 0x01
  Check = 2, //0x10
  Mate = 3 //0x11
}

return (GameState) (0x10 * isCheck + isStuck);
share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
13  
To all future developers, please don't ever use strings like this. –  Jeff Mercado Feb 18 '13 at 8:46
    
@JeffMercado: what's your objection? I don't want to defend it, it's not what I would typically implement. It really has nothing to recommend it except possibly readability for someone without experience with the alternatives. But it also doesn't seem to be an abuse of strings either. –  jmoreno Feb 19 '13 at 5:52
1  
The objection is clearly that the code is literally undefendable. It is inefficient and inelegant. –  Reacher Gilt Feb 20 '13 at 18:05
1  
Clear sign of the need to have pattern matching in C#. –  Rotsor Feb 20 '13 at 19:08
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.