Etydhv

This is the first little project I've made that didn't feel it was complete gibberish. But I couldn't tell.

The biggest problem I had with this was using the BoardValue enum working like I wanted to. I understand that classes should have a level of abstraction to them and I suspect that the way I implemented the at(int) returning a char over a BoardValue took away from that. However, I though having to convert the return from at(int) to a char if it returned a BoardValue would be redundant. For example, using a statement like this:

char print_char = Board.at(some_index) == BoardValue::o ? 'O' : 'X';

I hope I've done a decent job describing my dilemma.

Overall, I'm hoping for some overall general code style tips and pointers on how to write better code from here.

tictactoe.h

#ifndef TICTACTOE
#define TICTACTOE
#include <array>
#include <iostream>

enum BoardValue : char none = ' ', o = 'O', x = 'X' ;

class Board

public:
 Board()
 
 for(auto begin = board.begin(),end = board.end();begin != end; ++begin)
 *begin = BoardValue::none;
 

 char at(int index) const return board.at(index); 
 inline bool check_win(BoardValue) const;
 bool place(int, BoardValue);
private:
 bool check_diagonals(BoardValue) const;
 bool check_horizontals(BoardValue) const;
 bool check_verticals(BoardValue) const;

 std::array<char, 9> board;
;

inline bool Board::check_win(BoardValue check) const


#endif

tictactoe.cpp

#include "tictactoe.h"
#include <iostream>

//returns false if index is occupied
bool Board::place(int index, BoardValue value)

 if(board.at(index) != BoardValue::none)
 return false;
 board.at(index) = value;
 return true;


bool Board::check_diagonals(BoardValue check) const

 //if middle is not check no diagnols will pass
 if(board.at(4) != check)
 return false;
 //backward diagonal ''
 if(board.at(0) == check && board.at(4) == check)
 return true;
 //forward diaganol '/'
 if(board.at(2) == check && board.at(6) == check)
 return true;
 return false;


bool Board::check_horizontals(BoardValue check) const

 for(int row = 0; row < 3; ++row)
 if(board.at(row) == check &&
 board.at(row + 3) == check &&
 board.at(row + 6) == check)
 return true;
 
 return false;


bool Board::check_verticals(BoardValue check) const

 for(int col = 0; col < 3; ++col)
 if(board.at(col * 3) == check &&
 board.at(col * 3 + 1) == check &&
 board.at(col * 3 + 2 ) == check)
 return true;
 
 return false;

main.cpp

#include "tictactoe.h"
#include <iostream>

int ask_input(char player, bool retry = false)

 if(!retry)
 std::cout << "It's " << player
 << "'s turn. Where do you want to go(e.g. A1 B3 C2)? ";
 else
 std::cout << "No, no, no " << player
 << "! Input a letter followed bt a number: ";
 std::string input;
 std::cin >> input;

 if(input.size() < 2)
 return ask_input(player, true);

 int col_input;
 switch(*input.begin())
 
 case 'A':
 case 'a':
 col_input = 0;
 break;
 case 'B':
 case 'b':
 col_input = 1;
 break;
 case 'C':
 case 'c':
 col_input = 2;
 break;
 default:
 return ask_input(player, true);
 

 int row_input = *(input.begin() + 1) - '0'; //convers char '1' to int 1
 --row_input;

 return col_input * 3 + row_input;


BoardValue ask_turn() //ask whos first if return true O goes first

 BoardValue turn;
 std::string input;
 std::cout << "Who goes first(X or O)? ";
 for(bool valid_inputfalse; !valid_input;)
 
 std::cin >> input;
 switch(input.front()) //input cannot be null at this point
 
 case 'x':
 case 'X':
 valid_input = true;
 turn = BoardValue::x;
 break;
 case '0':
 case 'o':
 case 'O':
 valid_input = true;
 turn = BoardValue::x;
 break;
 default:
 std::cout << "Invalid input! Try X or O :";
 
 
 return turn;


std::ostream &print_board(std::ostream &os,const Board &board)
B

int main()
 Board board;
 BoardValue turn ask_turn() ; 
 //turn will be set back to appropriate value at start of game loop
 turn = turn == BoardValue::o ? BoardValue::x : BoardValue::o;
 int turn_count0;
 while(board.check_win(turn) == false)
 
 turn = turn == BoardValue::o ? BoardValue::x : BoardValue::o;
 print_board(std::cout, board);
 bool input_validfalse;
 while(input_valid == false)
 
 int input;
 input = ask_input(turn);
 input_valid = board.place(input, turn);
 if( input_valid == false )
 std::cout << "That place is take! Try again..n";
 
 if(++turn_count == 9) //max amount of turns game is tie
 break;
 
 print_board(std::cout, board);
 if(turn_count == 9)//game is tie
 std::cout << "Looks like its a tie...n";
 else
 std::cout << (char)turn << " wins!n";

Here are some things that may help you improve your code.

Use the required #includes

The code uses std::string which means that it should #include <string>. It was not difficult to infer, but it helps reviewers if the code is complete.

Have you run a spell check on comments?

If you run a spell check on your comments, you'll find a number of things such as "diagnols" and "diaganol" instead of "diagonals" and "diagonal." Since your code is nicely commented, it's worth the extra step to eliminate spelling errors.

Be wary of recursive calls

The ask_input routine has a subtle flaw. In particular, because it is recursive, it may be possible for a malicious user to crash the program by exhausting the stack. All that would be required would be to continue to input improperly formatted data. For this reason, as well as to make the code more understandable, I'd suggest instead to make retry a local variable and use that, as in a while loop, to re-ask if needed.

Fix the bug

The ask_input has a not-so-subtle flaw as well. It checks the letter, but not the number, so a user could input C9 or A0 and the program would attempt to use that!

Don't use std::endl if you don't really need it

The difference betweeen std::endl and 'n' is that 'n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when 'n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Be judicious with the use of inline

If a function is small and time critical, it makes sense to declare it inline. However, the check_win function is not really time critical, so I would say that there's no reason to make it inline.

Consider using a stream inserter

The existing print_board function is written exactly as one would write as one would write a stream inserter. The only thing that would change would be the declaration:

std::ostream &operator<<(std::ostream& os, const Board& board) /* ... */ 

Simplify your constructor

The Board constructor is currently defined like this:

Board()

 for(auto begin = board.begin(),end = board.end();begin != end; ++begin)
 *begin = BoardValue::none;

There are at least three ways to simplify it. One would be to use a "range-for" syntax:

Board()

 for(auto space : board) 
 space = BoardValue::none;
 

Another would be use fill:

Board() 
 board.fill(BoardValue::none);

A third way would allow you omit the constructor entirely. Do that by using aggregate initialization in the declaration of board:

std::array<char, 9> board
 ' ',' ',' ',
 ' ',' ',' ',
 ' ',' ',' ',
;

Think carefully about the class design

The structure of the code is not bad, but some things to think about are what things should be the responsibility of the Board class and which are not. For example, I think it might make more sense for Board to keep track of the number of turns.

Simplify the code

This line is not easy to read or understand:

turn = turn == BoardValue::o ? BoardValue::x : BoardValue::o;

I would suggest instead having turn be a bool that represents O. Then flipping back and forth would simply be turn = !turn;.