So Im editing a game off 3d tiktactoe and the switch statements are super long and i know there's a way to shorten it but i don't know how to go about it. the switch statements basically switch between turns of the player and the computer and they are tied to the spots on the board. here's the code:
bool D3Board::square_choose(bool player_Choice, int selection){
if(player_Choice)
{
// player turn
switch(selection)
{
case 1:
{
return square1.player_Choice();
break;
}
case 2:
{
return square2.player_Choice();
break;
}
case 3:{
return square3.player_Choice();
break;
}
case 4:{
return square4.player_Choice();
break;
}
case 5:{
return square5.player_Choice();
break;
}
case 6:{
return square6.player_Choice();
break;
}
case 7:{
return square7.player_Choice();
break;
}
case 8:{
return square8.player_Choice();
break;
}
case 9:{
return square9.player_Choice();
break;
}
case 10:{
return squareA1.player_Choice();
break;
}
case 11:{
return squareA2.player_Choice();
break;
}
case 12:{
return squareA3.player_Choice();
break;
}
case 13:{
return squareA4.player_Choice();
break;
}
case 14:{
return squareA5.player_Choice();
break;
}
case 15:{
return squareA6.player_Choice();
break;
}
case 16:{
return squareA7.player_Choice();
break;
}
case 17:{
return squareA8.player_Choice();
break;
}
case 18:{
return squareA9.player_Choice();
break;
}
case 19:{
return squareB1.player_Choice();
break;
}
case 20:{
return squareB2.player_Choice();
break;
}
case 21:{
return squareB3.player_Choice();
break;
}
case 22:{
return squareB4.player_Choice();
break;
}
case 23:{
return squareB5.player_Choice();
break;
}
case 24:{
return squareB6.player_Choice();
break;
}
case 25:{
return squareB7.player_Choice();
break;
}
case 26:{
return squareB8.player_Choice();
break;
}
case 27:{
return squareB9.player_Choice();
break;
}
default:
{
return 0;
}
}
}
else{
switch(selection)
{
case 1:{
return square1.comp_Choice();
break;
}
case 2:{
return square2.comp_Choice();
break;
}
case 3:{
return square3.comp_Choice();
break;
}
case 4:{
return square4.comp_Choice();
break;
}
case 5:{
return square5.comp_Choice();
break;
}
case 6:{
return square6.comp_Choice();
break;
}
case 7:{
return square7.comp_Choice();
break;
}
case 8:{
return square8.comp_Choice();
break;
}
case 9:{
return square9.comp_Choice();
break;
}
case 10:{
return squareA1.comp_Choice();
break;
}
case 11:{
return squareA2.comp_Choice();
break;
}
case 12:{
return squareA3.comp_Choice();
break;
}
case 13:{
return squareA4.comp_Choice();
break;
}
case 14:{
return squareA5.comp_Choice();
break;
}
case 15:{
return squareA6.comp_Choice();
break;
}
case 16:{
return squareA7.comp_Choice();
break;
}
case 17:{
return squareA8.comp_Choice();
break;
}
case 18:{
return squareA9.comp_Choice();
break;
}
case 19:{
return squareB1.comp_Choice();
break;
}
case 20:{
return squareB2.comp_Choice();
break;
}
case 21:{
return squareB3.comp_Choice();
break;
}
case 22:{
return squareB4.comp_Choice();
break;
}
case 23:{
return squareB5.comp_Choice();
break;
}
case 24:{
return squareB6.comp_Choice();
break;
}
case 25:{
return squareB7.comp_Choice();
break;
}
case 26:{
return squareB8.comp_Choice();
break;
}
case 27:{
return squareB9.comp_Choice();
break;
}
default:{
return 0;
}
}
}
}
CodePudding user response：
Place the squareXY in a container such that their index corresponds to the case label, then
if(player_Choice) {
return squares[selection].player_Choice();
} else {
return squares[selection].comp_Choice();
}
If the squareXY must be stored elsewhere for some reason, then squares can store pointers to the actual instances (-> return squares[selection]->player_Choice()).
In general (probably not in this particular case) a std::unordered_map or std::map can come in handy. For example when the lookup is not based on a contiguous unsigned integer. Just be aware of the tradeoff in memory and time complexity for look up (std::map stores a bit more than only the elements and has O(logn) lookup compared to O(n) with a simple array).
CodePudding user response：
While staying with switch you can remove in each case whatever is after return. It won't be executed anyway.
But it looks like your code could use an appropriatly filled array and then replace all of the switch with
return ArrayOfSquares[selection].playerChoice();
As the answer by not a number made me aware (credits), I should point out that you'd probably want to init the array[0] specially and you might still need an if for higher values.
CodePudding user response：
Yes indeed, and very easily.
Store all your squares into an array and call the instance method directly by indexing over it. Your code would simplify to.
if(player_choice)
return squares[selection].player_choice();
else
return squares[selection].comp_choice();
CodePudding user response：
One major improvement you could do in your design is to avoid having independent square and prefer create a vector (or an array, or a map) of squares. That will allow you to access the squares with an index.
std::vector<MySquare> squares;
squares.push_back(MySquare());
squares.push_back(MySquare());
squares.push_back(MySquare());
...
// Later on, in your switch statement:
square[2].player_Choice();
square[45].player_Choice();
CodePudding user response：
Or to make this even shorter and a lot more readable and beginner friendly:
First put your methods in an array:
using fptr = bool(Square::*)()const;
static constexpr fptr F[2] = {&Square::comp_Choice,&Square::player_Choice};
Then have some array for your squares somewhere:
Square squares[27];
And now it gets as beautiful as this:
bool square_choose(bool p, int i)
{
// no need for comments here, it's obvious
return (squares[i].*(F[p?1:0]))();
}
CodePudding user response：
- Create 2 lists of squares of length 27 each and that too one for each player.
- Now Upon user selection act on the desired list of squares.
var player_Squares = new Square[27];
var computer_Squares = new Square[27];
bool square_choose(bool player_Choice, int selection)
{
if(selection< 1 || selection > 27)
return false;
if(player_Choice)
{
return player_Squares[selection-1].Choice();
}
else
{
return computer_Squares[selection-1].Choice();
}
}