Strategy game move involving countries and dice

Question

I have a game that consists of multiple countries and each of them has several choices.

For instance, First Country(France) has multiple choices, Choice 1, Choice 2, Choice 3, .......etc

And the idea of the code is that if France gets number 6(the dice´result) it gets one (choice)soldier of its multiple ones into the game(exactly at the starting point so that contributes into the game).

And as it is shown each choice would make the code longer and longer .... Since i have multiple countries with multiple choices.

I am trying to improve my current code so that its easy to read. So the current code works well but i want to improve it and short it, maybe if i can avoid the repetition show that would significantly reduce.

So is there a smarter way to implement that so that it works as now but with a shorten type ?

bool Game::Move(string choice, int rrr)
{
    if (choice == "Choice1")
    {
        if (Choice1 == francestart)
        {
            if (rrr == 6) 
            {
                if (Choice1->next == Choice2 || Choice1->next == Choice3 || Choice1->next == Choice4) //if piece would land on space where another piece of the same country is occupying
                {
                    cout << "You cannot move " << (choice) << " out of the start position because one of your other pieces is occupying the space" << endl;
                    return false;
                }
                else
                {
                    Choice1 = Choice1->next;
                    Rest(choice);
                    return true;
                }   
            }   
            else
            {
                cout << "You cannot move " << (choice) << " " << " out of the start position because you did not rrr a 6" << endl;
                return false;
            }
        }
        else
        {
            Node* tttt;
            tttt = Choice1;
            bool ReturnBack = false; //tttt will go towards home country if true
            for (int i = 0; i < rrr; i++)
            {
                if (tttt->value == 90) //last space before going home, tells it to go to home space
                {
                    ReturnBack = true;
                }
                if (!ReturnBack)
                {
                    tttt = tttt->next;
                }
                if (ReturnBack)
                {
                    tttt = tttt->out;
                }
                if (tttt == NULL) //if run out of spaces
                {
                    cout << "You cannot move " << (choice) << " " << (rrr) << " spaces because there is not enough spaces to travel" << endl;
                    return false;
                }
            }
            if (tttt == Choice2 || tttt == Choice3 || tttt == Choice4) //if space is occupied
            {
                cout << "You cannot move " << (choice) << " " << (rrr) << " spaces because another one of your pieces is occupying that space" << endl;
                return false;
            }
            else
            {
                Choice1 = tttt;
                Rest(choice);
                return true;
            }   
        }
    }

    if (choice == "Choice2")
    {
        if (Choice2 == francestart)
        {
            if (rrr == 6) 
            {
                if (Choice2->next == Choice1 || Choice2->next == Choice3 || Choice2->next == Choice4) //if piece would land on space where another piece of the same country is occupying
                {
                    cout << "You cannot move " << (choice) << " out of the start position because one of your other pieces is occupying the space" << endl;
                    return false;
                }
                else
                {
                    Choice2 = Choice21->next;
                    Rest(choice);
                    return true;
                }   
            }   
            else
            {
                cout << "You cannot move " << (choice) << " " << " out of the start position because you did not rrr a 6" << endl;
                return false;
            }
        }
        else
        {
            Node* tttt;
            tttt = Choice2;
            bool ReturnBack = false; //tttt will go towards home country if true
            for (int i = 0; i < rrr; i++)
            {
                if (tttt->value == 24) //last space before going home, tells it to go to home space
                {
                    ReturnBack = true;
                }
                if (!ReturnBack)
                {
                    tttt = tttt->next;
                }
                if (ReturnBack)
                {
                    tttt = tttt->out;
                }
                if (tttt == NULL) //if run out of spaces
                {
                    cout << "You cannot move " << (choice) << " " << (rrr) << " spaces because there is not enough spaces to travel" << endl;
                    return false;
                }
            }
            if (tttt == Choice1 || tttt == Choice3 || tttt == Choice4) //if space is occupied
            {
                cout << "You cannot move " << (choice) << " " << (rrr) << " spaces because another one of your pieces is occupying that space" << endl;
                return false;
            }
            else
            {
                Choice2 = tttt;
                Rest(choice);
                return true;
            }   
        }
    }

And i still have

if (choice == "Choice1")
    {
        if (Choice1 == italystart)
        {
            if (rrr == 6) 

And so on ?

Answer 1

I see several things that can be factored out into more generic functions and which would help reduce the amount of code you're writing.

Refactoring

Your code comes down to 2 if blocks which have the same basic structure. So a good place to start is to factor out what's within those if blocks. That will leave the Move() method looking like this:

bool Game::Move(string choice, int rrr)
{
    if (choice == "Choice1")
    {
        return HandleChoice (choice, Choice1, francestart, rrr);
    }
    if (choice == "Choice2")
    {
        return HandleChoice (choice, Choice2, italystart, rrr);
    }
}

This mysterious new HandleChoice() method must do what used to be inside the if blocks. As mentioned, they both have the same basic structure which is that they also check a value and do one thing if it's equal and another if it's not. So it will look something like this:

bool Game::HandleChoice(string choice, Node*& chosenNode, Node* country, int rrr)
{
    if (chosenNode == country)
    {
        if (rrr == 6)
        {
            return HandleRoll(choice, chosenNode, chosenNode->next);
        }
        else
        {
            cout << "You cannot move " << (choice) << " " << " out of the start position because you did not rrr a 6" << endl;
            return false;
        }
    }
    else
    {
        Node* tttt;
        tttt = Choice1;
        bool ReturnBack = false; //tttt will go towards home country if true
        for (int i = 0; i < rrr; i++)
        {
            if (tttt->value == 90) //last space before going home, tells it to go to home space
            {
                ReturnBack = true;
            }
            if (!ReturnBack)
            {
                tttt = tttt->next;
            }
            if (ReturnBack)
            {
                tttt = tttt->out;
            }
            if (tttt == NULL) //if run out of spaces
            {
                cout << "You cannot move " << (choice) << " " << (rrr) << " spaces because there is not enough spaces to travel" << endl;
                return false;
            }
        }
        return HandleRoll(choice, chosenNode, tttt);
    }
}

The HandleRoll() function takes the functionality that's the same between the 2 if blocks in HandleChoice() and makes it into a single method like this:

bool Game::HandleRoll(string choice, Node*& chosenNode, Node* nextNode)
{
    if (ChoiceTaken(chosenNode))
    {
        cout << "You cannot move " << (choice) << " out of the start position because one of your other pieces is occupying the space" << endl;
        return false;
    }
    else
    {
        chosenNode = nextNode;
        Rest(choice);
        return true;
    }
}

That leaves the task of writing the method ChoiceTaken(). This one is tricky because it's not clear what the intent is. It appears that you want to check that the given Node is not the same as any one of 3 other nodes. You could pass the other 3 nodes all the way through the function calls to the ChoiceTaken() method and do the comparison there, but that seems like a lot of data to pass around. Also, it looks like the 4 choice nodes are member variables, so you shouldn't have to pass them around since the object already has access to them. If, instead of 4 separate variables you had an array of 4 values, you could simply iterate through the array and ensure that the new Choice* doesn't match any in the array (assuming it can't also equal its current value). But since we don't know what your class looks like, it's hard to advise how to do that.

Here's what I'm saying. You could pass in all the values you want to check against like this:

bool Game::ChoiceTaken(Node* usersChoice, Node* checkNode1, Node* checkNode2, Node* checkNode3)
{
    if ((usersChoice == checkNode1->next) || (usersChoice == checkNode2->next) || (usersChoice == checkNode3->next))
    {
        return true;
    }
    return false;
}

Where checkNode1, checkNode2, and checkNode3 would be Choice2, Choice3, and Choice4 when the user's choice is "Choice1". when it's "Choice2" you would pass 1, 3, and 4. Etc. That's a lot of values to pass through from Game::Move() all the way to Game::ChoiceTaken(). So it might be easier if you have them in an array already within the Game object to do something like this:

bool Game::ChoiceTaken(Node* usersChoice)
{
    for (int i = 0; i < kNumChoices; i++)
    {
        if (usersChoice == AllChoices [ i ]->next)
        {
            return true;
        }
    }
    return false;
}

In this scenario, you have a constant named kNumChoices that equals the number of choices available. (4 in your code above.) And there's an array that is a member variable in the Game object named AllChoices which contains Choice1, Choice2, etc. That will only work if the user is not allowed to make the choice that is already chosen. Otherwise you'll need to add some logic to skip checking against the current choice.

Naming

The variable names in your code are difficult to understand. It looks like maybe rrr is supposed to be the result of the roll of a 6-sided die, but I'm not sure. If it is, dieRoll would be a much better name. It's not clear at all what tttt is supposed the represent. (A temporary pointer to a node?) You have a member named simply value. What does the value represent? You have used the word choice in at least 2 different ways - once as a string and once as a Node. (Also what kind of node is a Node?) It looks like the node version represents a country of some sort. Perhaps using the word country in the name of the variable would be a better choice?

Magic Numbers

There are several magic numbers in your code whose meaning is unclear. rrr is compared to 6. Why? What does 6 represent? tttt->value is compared to 90. 90 what? The fact that you have to put a comment next to it describing what it is is an indication that you should have a descriptive name for it. For example, it could be a named constant called lastSpace or returnSpace or something along those lines.

Answer 2

I find the description very unclear, and the code is only a small fragment, making it difficult to figure out what you're asking. Here's my guess as to what you're trying to do:

1. each player is a country
2. each player rolls a single die
3. the player can then choose a single soldier to move
4. invalid moves must be rejected
5. a valid move should perform the requested move

If that's correct, first I'd write a Die class to provide a convenient means of getting that.

#include <random> 

class Die {
public:
    int operator()() { 
        std::uniform_int_distribution<> dist(1,6);
        return dist(eng);
    }

private:
    static std::mt19937 eng;
};

std::mt19937 Die::eng{std::random_device{}()};

Next, a turn should probably be a function of the Country rather than the game. Assuming there is a fixed array of players and that they each take a turn in order, here's an outline of how that might work:

Country player{"France", "Italy", "Spain", "Germany"};
Die roll;
for (auto &p : player) {
    p.move(roll());
}

The Country class would then need a member function called move that would present the die roll value and ask the user for a choice of what to do. It should only return when a valid move has been executed. It would also likely be useful to have a helper function:

bool Country::isValidMove(int soldiernum, int destination) const;

I'll leave it to you to write the details of those functions, but the general advice is: Don't Repeat Yourself (DRY). When you find yourself writing line after line of extremely repetitive code, it's a strong sign that you should be restructuring you code to eliminate such repetition.