• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Bardziej "czysta" wersja kodu

0 głosów
168 wizyt
pytanie zadane 7 sierpnia 2018 w C i C++ przez Huberti Gaduła (4,520 p.)
edycja 7 sierpnia 2018 przez Huberti

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;
}
1
komentarz 7 sierpnia 2018 przez Tomek Sochacki Ekspert (228,960 p.)

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.

komentarz 8 sierpnia 2018 przez Huberti Gaduła (4,520 p.)

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';
}
2
komentarz 8 sierpnia 2018 przez Tomek Sochacki Ekspert (228,960 p.)

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" 

 

komentarz 8 sierpnia 2018 przez Huberti Gaduła (4,520 p.)

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);
komentarz 8 sierpnia 2018 przez Tomek Sochacki Ekspert (228,960 p.)
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!

Zaloguj lub zarejestruj się, aby odpowiedzieć na to pytanie.

Podobne pytania

0 głosów
2 odpowiedzi 130 wizyt
0 głosów
1 odpowiedź 281 wizyt
pytanie zadane 23 grudnia 2017 w C i C++ przez Huberti Gaduła (4,520 p.)
0 głosów
2 odpowiedzi 333 wizyt
pytanie zadane 19 października 2015 w HTML i CSS przez Javowiec Pasjonat (21,580 p.)

88,661 zapytań

137,269 odpowiedzi

306,597 komentarzy

58,863 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Sklep oferujący ćwiczenia JavaScript, PHP, rozmowy rekrutacyjne dla programistów i inne materiały

Oto dwie polecane książki warte uwagi. Pełną listę znajdziesz tutaj.

...