4
\$\begingroup\$

This is my first program using classes. How could this be improved?

#include <iostream>

class TicTacToe
{
    char m_field[3][3];
    char sign;
public:
    char Sign(char x) {sign=x;};
    void MakeField();
    void DrawField();
    bool IsFree(int y, int x);
    bool FindAndReplace(int x,int y);
    bool Row();
    bool Column();
    bool Diagonal();
    bool Win();
    bool IsFull();
    TicTacToe(){MakeField();DrawField();}
};

void TicTacToe::MakeField()
    {
        for (int i = 0; i < 3; i++)
            for(int j = 0; j < 3; j++)
                m_field[i][j]=' ';
    }

void TicTacToe::DrawField()
    {
        std::cout << " ";
        for(int i = 0; i < 3; i++)
            std::cout << " " << i;

        std::cout << "\n  - - -\n";
        for(int i = 0; i < 3; i++)
        {
            std::cout << i;
            for(int j = 0; j < 3; j++)
            {
                std::cout << "|" << m_field[i][j];
            }
            std::cout << "|\n  - - -\n";
        }
    }

bool TicTacToe::IsFree(int y, int x)
    {
        if(m_field[y][x]==' ')
            return true;
        return false;
    }

bool TicTacToe::FindAndReplace(int x,int y)
    {
        if(IsFree(y,x))
        {
            m_field[y][x]=sign;
            return true;
        }
        return false;
    }

bool TicTacToe::Row()
    {
        for (int i = 0; i < 3; i++)
            if(!IsFree(i,0)&&!IsFree(i,1)&&!IsFree(i,2))
            if(m_field[i][0]==m_field[i][1]&&m_field[i][0]==m_field[i][2])
                return true;
        return false;
    }

bool TicTacToe::Column()
    {
        for (int i = 0; i < 3; i++)
            if(!IsFree(0,i)&&!IsFree(1,i)&&!IsFree(2,i))
            if(m_field[0][i]==m_field[1][i]&&m_field[0][i]==m_field[2][i])
                return true;
        return false;
    }

bool TicTacToe::Diagonal()
    {
        if(!IsFree(1,1))
        {
            if(m_field[2][0]==m_field[1][1]&&m_field[1][1]==m_field[0][2])
                return true;
                if(m_field[0][0]==m_field[1][1]&&m_field[0][0]==m_field[2][2])
                return true;
        }
        return false;
    }

bool TicTacToe::Win()
    {
        return Row()||Column()||Diagonal();
    }

bool TicTacToe::IsFull()
    {
        for (int i = 0; i < 3; i++)
            for(int j = 0; j < 3; j++)
                if(IsFree(i,j))
                    return false;
        return true;
    }

int main()
{
    int x,y,i=0;
    char sign;
    TicTacToe field;
    while(!field.IsFull())
    {
        i++;
        sign=i%2==0?'o':'x';
        std::cout << "Turn " << sign << ": Get x cordinate:";
        std::cin >> x;
        std::cout << "Get y cordinate:";
        std::cin >> y;
        if(x>2||y>2)
        {
            std::cout << "this coordinates does not exist! Try again\n";
            i--;
            continue;
        }

        field.Sign(sign);
        if(field.FindAndReplace(x,y))
            field.DrawField();
        else
        {
            std::cout << "is full!";
            i--;
        }
        if(field.Win())
        {
            std::cout << "Winned sign " << sign;
            break;
        }
    }
    std::cout << "\nEnd!\n";
    return 0;
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Some comments on naming and coding style.

First, it is a bit unusual for member function names to be in PascalCase in C++. camelCase or snake_case are more usual, reserving identifiers that start with an uppercase letter for type names.

char sign;
char Sign(char x) {sign=x;};
field.Sign(sign);

This isn't clear to me at all (could be a language issue though). What is this "sign" thing? Positive/negative? Does the Sign method do a cryptographic signature? What does Sign do to a sign?

I'd go with player, and rename TicTacToe::Sign to SetPlayer/SetCurrentPlayer to make it clear it's setting something.

Or get rid of that entirely and pass the current player's symbol to the methods that need it: there's only one. This lets the TicTacToe class to only manage the board, and leaves managing players to your future gameplay controller class ('cos stuffing all that in main isn't the best idea).

void MakeField();

The field is already "made", but not initialized. Something like InitializeField or ClearBoard would be sound more appropriate to me (though that's really minor).

void DrawField();           // good
bool IsFree(int y, int x);  // great
bool FindAndReplace(int x,int y);

This method doesn't find anything. It sets a cell to a value if it's free. And wait a sec, didn't we just see another method that takes coordinates?

bool IsFree        (int y, int x);  // oh?
bool FindAndReplace(int x, int y);  // what?

Why do you pass in the coordinates in different order? Please don't do that, it is very, very error prone. Stick to one order, and make that x, y consistently.

bool Row();
bool Column();
bool Diagonal();

These names don't tell me anything about what the functions do. I'd expect them to somehow return are row, column or diagonal, but bool doesn't work for any of this. Plus they operate on all rows, columns and diagonals (plural).

I'd rename all that to CheckRows() etc., and make them private - users aren't supposed to call these directly. Then fix the indentation, especially since you're not using {} in your ifs - your current layout is misleading.

bool Win();

IsGameWon()?

bool IsFull();

IsGameTied()?

TicTacToe(){MakeField();DrawField();}

The constructor initializes the field - that's good, but then it prints it immediately. This might not be what the caller wants, and the caller doesn't have a choice. I'd remove the DrawField() call, and add some whitespace.

TicTacToe() { MakeField(); }

Now in main, this should be in a separate function that does just that, i.e. get valid input from the player.

    std::cout << "Turn " << sign << ": Get x cordinate:";
    std::cin >> x;
    std::cout << "Get y cordinate:";
    std::cin >> y;
    if(x>2||y>2)
    {
        std::cout << "this coordinates does not exist! Try again\n";
        i--;
        continue;
    }

The i--; continue; is not very intuitive, and you're likely to get something wrong if you ever update the code and add something in between the top of the loop and the continue statement.

I personally don't like this form:

    if(field.FindAndReplace(x,y))
        field.DrawField();
    else
    {
        std::cout << "is full!";
        i--;
    }

I'd {} both sides, and only omit the braces only if both sides are single, simple statements. Most importantly be consistent - your Row, Col and Diagonal methods aren't (for the for).

As for spacing:

    sign=i%2==0?'o':'x';

This is unreadable for me (slightly better if I put my glasses on, but still hard to parse). More spaces, at least:

    sign = i%2==0 ? 'o' : 'x';

And I'd personally add a pair of parens and even more spaces, but I tend to like parens more than most I think.

    sign = (i % 2 == 0) ? 'o' : 'x';

Same applies for most of your assignments and comparisons, they could do with more spaces (and perhaps more parens), e.g.

if (!IsFree(0, i) && !IsFree(1, i) && !IsFree(2, i))
if ((m_field[0][i] == m_field[1][i]) && (m_field[0][i] == m_field[2][i]))
m_field[y][x] = sign;

Finally:

bool TicTacToe::Something()
    {
    }

but:

int main()
{
}

That's unusual. I'd stick to one style only - the class/struct name already informs the reader that it's a member function, so I see no reason for a different style between toplevel and member functions. (I'd pick the style you chose for main, less wasted horizontal space.)


A couple suggestions for the code itself.

This one is (I believe) non controversial:

bool TicTacToe::IsFree(int y, int x)
{
  // no need for an 'if', you have a boolean result already
  return m_field[y][x] == ' ';
}

This is a bit trickier but makes your row and column check a bit easier to read:

bool TicTacToe::CheckRows()
{
  for (int i = 0; i < 3; i++) {
    if (IsFree(i, 0)) // if the first cell is free, no need to look further
      continue;
    if ((m_field[i][0] == m_field[i][1]) && (m_field[i][0] == m_field[i][2]))
      return true; // and if it isn't the other two must match to win
  }
  return false;
}

This is a matter of taste - getting the "easy" cases out of the way as soon as possible generally results in less nesting depths for the main part of the function. (Here, not much gained.)

bool TicTacToe::SetIfFree(int x, int y)
{
  if (!IsFree(y, x))
    return false;

  m_field[y][x] = sign;
  return true;
}
\$\endgroup\$
2
  • \$\begingroup\$ whatever_this_is_called is frequently referred to as Snake Case. \$\endgroup\$
    – glampert
    Commented Jan 25, 2015 at 17:04
  • \$\begingroup\$ Thanks @glampert. Edited in (and added another remark I hadn't spotted). \$\endgroup\$
    – Mat
    Commented Jan 25, 2015 at 17:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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