Two-player Mancala

After playing Mancala with my brother, I thought coding the game would be a fun project to practice my abilities in C++. I am relatively new to programming, having only done a few small projects to experiment with the basic features of the language. So I consider this to be my first legitimate application of my skills.

Although proud of my result I figured it could be better, especially since I have a function for each player (p1 and p2) instead of a player class. The reason why is because I "translate" the inputs. Hopefully that makes sense when looking at the code. I tried to find other source code to compare to my game, but everything I have found so far does not make much use of C++ and is rather lengthy. I appreciate any advice, but would really enjoy a full answer as to how to improve my code and further my abilities.

My code is separated into three parts; a menu class, a match class, and a main file.

mancala.h

```
#ifndef MANCALA_H
#define MANCALA_H

#include
#include
#include

namespace mancala{

const std::vector rules = {
"\nOverview:\n\tMancala is an ancient family of board games,\n\tand there are numerous variants.\n\tThis is a version of the basic game,\n\tknown as two-rank Mancala and also known as Kalah.\n\t",
"\nHoles & Stores:\n\tYour holes on the board are on the bottom right side.\n\tYour store is the last hole on your right.\n\t",
"\nSow Stones:\n\tChoose a hole to pick up all pieces.\n\tMoving counter - clockwise, the game now deposits one of\n\tthe stones in each hole until the stones run out.\n\t",
"\nStore:\n\tIf you run into your own store deposit one piece in it.\n\tIf you run into your opponents store, skip it.\n\t",
"\nFree Turn:\n\tIf the last piece you drop is in your own store,\n\tyou get a free turn.\n\t",
"\nCapture:\n\tIf the last piece you drop is on your side,\n\tyou capture that piece and any pieces in the\n\thole directly opposite.\n\t",
"\nEnd:\n\tThe g

I have found a couple of things that could help you improve your code.

Avoid the use of global variables

I see that home is declared as global variable, but there's no reason it can't instead be a variable within main. It's generally better to keep the scope of variables as small as practical.

Fix the formatting

This line is badly formatted:

int main(){bool end = false;

Running things together like that makes your code hard to read and understand. I'd format it like this instead:

int main()
{
    bool end = false;
    // etc.

Don't use system("clear")

There are two reasons not to use system("clear"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named clear, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate this into a separate functions cls() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. An example using ANSI escape sequences:

void cls()
{
    std::cout << "\x1b[2J";
}

Eliminate spurious semicolons

There are a lot of spurious semicolons at the end of function definitions. For example:

int trans(const unsigned int& index){
        return (7 + index) % 14;
};  // <-- this semicolon is neither needed nor welcome!

Eliminating those will make your code a bit easier to read and understand.

Use a player object

As you've already noted, your code would benefit greatly from having a player object. If we look at p1_display and p2_display for example, only a single line is different in the most trivial way. This could easily be made into a member function of a player object.

Use const where practical

Several of your member functions, such as results() do not alter the underlying object. For that reason, those functions should be declared const like this:

void match::results() const { // etc.

Avoid strange constructions

Normally, a switch statement has multiple cases. However this code contains this odd bit of code:

switch(match::p2_clear_pits() || match::p1_is_greater() || match::p2_pit_distribute(match::p2_pit_select()) || match::p2_clear_pits() || match::p1_is_greater()){
    case true:
        end();
        return;
}

First, it's odd because it only has a single case. It could be an if statement instead. Second, it's hard to read because the line is so long. If we pull it apart, we get to the next strange part:

if (match::p1_clear_pits() || match::p2_is_greater() || 
        match::p1_pit_distribute(match::p1_pit_select()) || 
        match::p1_clear_pits() || match::p2_is_greater()) {  //etc.

The first and third lines are identical. Unfortunately, the p1_pit_distribute actually alters the state, so we can't simply eliminate the third line. This really needs to be refactored. I'd suggest a simple hasWon() function to determine if one or the other has won, and not to abuse short-circuit evaluation this way.

Separate I/O from program logic

Right now most individual functions have both game logic and I/O. It's often better design to separate the two so that the game logic is independent of the I/O with the user. That way if you decided, for instance, to make a GUI version, only the I/O parts would need to be touched. It also makes your code easier to understand and to fix, even if you never change the I/O.

Don't abuse an object constructor

The constructor for match looks like this:

match() : board{4,4,4,4,4,4,0,4,4,4,4,4,4,0}{
    begin();
    while(ingame)
        {match::update();}
    results();
};

This is not good design. Instead, separate the two concepts of construction and playing. It might look like this:

match() : board{4,4,4,4,4,4,0,4,4,4,4,4,4,0}
{}
void play() {
    while (update()) 
    { }
    results();
}

Note that ingame is eliminated entirely, and that update now returns true until the game is complete. This also, of course, completely eliminates the need for begin() and end() member functions as well.

Think of the user

The instructions are printed only two or three lines at a time. Unless your display is only expected to be three lines high, this makes it much more work for the user to read and understand the instructions as a while. For that reason, I'd recommend simply having the whole of the instructions as a single string rather than a vector of strings.

StackExchange Code Review Q#99033, answer score: 5