Bardziej "czysta" wersja kodu

Question

Witam, która z poniższych wersji kodu jest bardziej "czysta"? Przy okazji proszę rownież o ocenę kodu. 

Jest to plik GameModes.cpp z gry kółko i krzyżyk. Wrzuciłem cały, aby lepiej zobrazować sytuację.

Wersja 1 (kod bezpośrednio w funkcji, co powoduje usunięcie podwójnego sprawdzenia czyja jest teraz tura):

#include "GameModes.h"
#include "TicTacToe.h"
#include "ManagingNamesOfPlayers.h"

resultOfGame makePlayerVsComputerGame(const std::string &nameOfPlayer)
{
	char gameBoard[9]{ '1', '2', '3', '4', '5', '6', '7', '8', '9' };
	std::string whoseTurn = getRandomlyWhoStartsGame();
	int indexOfCellWithoutProprietorOfGameBoard;

	system("cls");
	if (isCircleTurn(whoseTurn))
	{
		std::cout << "Twoja tura " << nameOfPlayer << " (O)" << std::endl;
		showGameBoard(gameBoard);
		indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
	}	
	else
	{
		std::cout << "Tura przeciwnika (X)" << std::endl;
		showGameBoard(gameBoard);
		indexOfCellWithoutProprietorOfGameBoard = getRandomIndexOfCellWithoutProprietorOfGameBoard(gameBoard);
		std::cout << "Wybieram komorke z numerem " << indexOfCellWithoutProprietorOfGameBoard + 1 << std::endl;
		waitTwoSecounds();
	}
	setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);

	while (!isEndGame(gameBoard))
	{
		setTurnToTheNextPlayer(whoseTurn);
		system("cls");	
		if (isCircleTurn(whoseTurn))
		{
			std::cout << "Twoja tura " << nameOfPlayer << " (O)" << std::endl;
			showGameBoard(gameBoard);
			indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
		}		
		else
		{
			std::cout << "Tura przeciwnika (X)" << std::endl;
			showGameBoard(gameBoard);
			indexOfCellWithoutProprietorOfGameBoard = getRandomIndexOfCellWithoutProprietorOfGameBoard(gameBoard);
			std::cout << "Wybieram komorke z numerem " << indexOfCellWithoutProprietorOfGameBoard + 1 << std::endl;
			waitTwoSecounds();
		}
		setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);
	}

	system("cls");
	showGameBoard(gameBoard);
	if (isTie(gameBoard))
	{
		std::cout << "Remis.";
		return GAME_WITHOUT_WINNER;
	}		
	else if (isCircleTurn(whoseTurn))
	{
		std::cout << "Wygrana" << std::endl;
		return FIRST_PLAYER_WON_GAME;
	}		
	else
	{		
		std::cout << "Przegrana" << std::endl;
		return FIRST_PLAYER_LOST_GAME;
	}
}

resultOfGame makePlayerVsPlayerGame(const NamesOfPlayers &namesOfPlayers)
{
	char gameBoard[9]{ '1', '2', '3', '4', '5', '6', '7', '8', '9' };
	std::string whoseTurn = getRandomlyWhoStartsGame();
	int indexOfCellWithoutProprietorOfGameBoard;

	system("cls");
	if (isCircleTurn(whoseTurn))
		std::cout << "Tura gracza " << namesOfPlayers.nameOfFirstPlayer << " (O)" << std::endl;
	else
		std::cout << "Tura gracza " << namesOfPlayers.nameOfSecoundPlayer << " (X)" << std::endl;
	showGameBoard(gameBoard);
	indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
	setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);

	while (!isEndGame(gameBoard))
	{
		setTurnToTheNextPlayer(whoseTurn);
		system("cls");
		if (isCircleTurn(whoseTurn))
			std::cout << "Tura gracza " << namesOfPlayers.nameOfFirstPlayer << " (O)" << std::endl;
		else
			std::cout << "Tura gracza " << namesOfPlayers.nameOfSecoundPlayer << " (X)" << std::endl;
		showGameBoard(gameBoard);
		indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
		setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);
	}

	system("cls");
	showGameBoard(gameBoard);
	if (isTie(gameBoard))
	{
		std::cout << "Remis" << std::endl;
		return GAME_WITHOUT_WINNER;
	}		
	else if (isCircleTurn(whoseTurn))
	{
		std::cout << "Wygrywa gracz " << namesOfPlayers.nameOfFirstPlayer << std::endl;
		return FIRST_PLAYER_WON_GAME;
	}		
	else
	{
		std::cout << "Wygrywa gracz " << namesOfPlayers.nameOfSecoundPlayer << std::endl;
		return FIRST_PLAYER_LOST_GAME;
	}		
}

Wersja 2 (kod umieszczony w osobnej funkcji, co powoduje zmniejszenie funkcji, ale kosztem podwójnego sprawdzenia czyja jest teraz tura):

#include "GameModes.h"
#include "TicTacToe.h"
#include "ManagingNamesOfPlayers.h"

resultOfGame makePlayerVsComputerGame(const std::string &nameOfPlayer)
{
	char gameBoard[9]{ '1', '2', '3', '4', '5', '6', '7', '8', '9' };
	std::string whoseTurn = getRandomlyWhoStartsGame();
	int indexOfCellWithoutProprietorOfGameBoard;

	system("cls");
	showMessageWhoseTurnIsNow(whoseTurn, nameOfPlayer);
	showGameBoard(gameBoard);
	if (isCircleTurn(whoseTurn))
		indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
	else
	{
		indexOfCellWithoutProprietorOfGameBoard = getRandomIndexOfCellWithoutProprietorOfGameBoard(gameBoard);
		std::cout << "Wybieram komorke z numerem " << indexOfCellWithoutProprietorOfGameBoard + 1 << std::endl;
		waitTwoSecounds();
	}
	setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);

	while (!isEndGame(gameBoard))
	{
		setTurnToTheNextPlayer(whoseTurn);
		system("cls");
		showMessageWhoseTurnIsNow(whoseTurn, nameOfPlayer);
		showGameBoard(gameBoard);
		if (isCircleTurn(whoseTurn))
			indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
		else
		{
			indexOfCellWithoutProprietorOfGameBoard = getRandomIndexOfCellWithoutProprietorOfGameBoard(gameBoard);
			std::cout << "Wybieram komorke z numerem " << indexOfCellWithoutProprietorOfGameBoard + 1 << std::endl;
			waitTwoSecounds();
		}
		setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);
	}

	system("cls");
	showGameBoard(gameBoard);
	if (isTie(gameBoard))
	{
		std::cout << "Remis.";
		return GAME_WITHOUT_WINNER;
	}		
	else if (isCircleTurn(whoseTurn))
	{
		std::cout << "Wygrana" << std::endl;
		return FIRST_PLAYER_WON_GAME;
	}		
	else
	{		
		std::cout << "Przegrana" << std::endl;
		return FIRST_PLAYER_LOST_GAME;
	}
}

resultOfGame makePlayerVsPlayerGame(const NamesOfPlayers &namesOfPlayers)
{
	char gameBoard[9]{ '1', '2', '3', '4', '5', '6', '7', '8', '9' };
	std::string whoseTurn = getRandomlyWhoStartsGame();
	int indexOfCellWithoutProprietorOfGameBoard;

	system("cls");
	showMessageWhoseTurnIsNow(whoseTurn, namesOfPlayers);
	showGameBoard(gameBoard);
	indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
	setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);

	while (!isEndGame(gameBoard))
	{
		setTurnToTheNextPlayer(whoseTurn);
		system("cls");
		showMessageWhoseTurnIsNow(whoseTurn, namesOfPlayers);
		showGameBoard(gameBoard);
		indexOfCellWithoutProprietorOfGameBoard = getIndexOfCellOfGameBoardFromInputStreamWhileCellOfThatIndexHasProprietor(gameBoard);
		setCellOfGameBoardToBePropertyOfCurrentPlayer(gameBoard[indexOfCellWithoutProprietorOfGameBoard], whoseTurn);
	}

	system("cls");
	showGameBoard(gameBoard);
	if (isTie(gameBoard))
	{
		std::cout << "Remis" << std::endl;
		return GAME_WITHOUT_WINNER;
	}		
	else if (isCircleTurn(whoseTurn))
	{
		std::cout << "Wygrywa gracz " << namesOfPlayers.nameOfFirstPlayer << std::endl;
		return FIRST_PLAYER_WON_GAME;
	}		
	else
	{
		std::cout << "Wygrywa gracz " << namesOfPlayers.nameOfSecoundPlayer << std::endl;
		return FIRST_PLAYER_LOST_GAME;
	}		
}

Fragment z pliku TicTacToe.cpp

void showMessageWhoseTurnIsNow(const std::string &whoseTurn, const std::string &nameOfPlayer)
{
	if (whoseTurn == "circle")
		std::cout << "Twoja tura " << nameOfPlayer << " (O)" << std::endl;
	else
		std::cout << "Tura przeciwnika (X)" << std::endl;
}

void showMessageWhoseTurnIsNow(const std::string &whoseTurn, const NamesOfPlayers &namesOfPlayers)
{
	if (whoseTurn == "circle")
		std::cout << "Tura gracza " << namesOfPlayers.nameOfFirstPlayer << " (O)" << std::endl;
	else
		std::cout << "Tura gracza " << namesOfPlayers.nameOfSecoundPlayer << " (X)" << std::endl;
}

Comments

Nie znam się na C więc samego kodu nie ocenię, ale...

setCellOfGameBoardToBePropertyOfCurrentPlayer

to nie jest fajna nazwa... za długa... Owszem, należy tworzyć jasne i samokomentujące się nazwy funkcji, zmiennych itp. ale nie można przesadzać :)

Jeśli mimo wszystko wg Ciebie taka nazwa na prawdę ma sens aby wyrazić to co funkcja robi to zastanów się, czy nie lepiej rozbić tę funkcjonalność na mniejsze.

Poczytaj też np. o TDD, czyli zasadzie najpierw testy, potem kod. Nie chodzi o to abyś od razu wprowadzał to w życie, ale żebyś zaczął więcej myśleć o przyszłościowych testach co wpłynie na pewno na jakość Twojego kodu.

---

Czy istnieje sposób, aby rozbić tę funkcję na mniejsze?

void setCellOfGameBoardToBePropertyOfCurrentPlayer(char &cell, const std::string &whoseTurn)
{
	if (whoseTurn == "circle")
		cell = 'O';
	else
		cell = 'X';
}

---

Rozbić w sumie nie, ale nazwa na pewno jest zła... jak dojdziesz do końca to zapomnisz co było w nazwie na początku... a jak w kodzie będzie takich dużo to nawet z IDE i podpowiadaniem tego nie ogarniesz...

a tak w ogóle to nie lepiej to zapisać krócej jako:

cell = (whoseTurn == "circle") ? "O" : "X" 

 

---

Dzięki, faktycznie tak wygląda to lepiej.

Właściwie to można jeszcze napisać tak:

char getSymbolOfCurrentPlayer(const std::string &whoseTurn)
{
    return (whoseTurn == "circle") ? 'O' : 'X';
}

Wtedy wywołanie:

gameBoard[indexOfCell] = getSymbolOfCurrentPlayer(whoseTurn);

---

I to się nazywa Kolego refaktoryzacja kodu... tutaj zaczyna się wg mnie różnica między "gościem po bootcampie", a kimś, kto faktycznie chce być programistą co widać u Ciebie.

Także działaj dalej i na spokojnie poprzeglądaj swój kod bo widzę, że szybko załapiesz wiele dobrych praktyk i sam do wielu rzeczy dojdziesz :) Gdy to zrobisz to możesz też powoli wchodzić w zagadnienia np. wzorców projektowych i sam zobaczysz jak jeszcze można niektóre problemy zmodyfikować :)

Pozdrawiam i powodzenia!