Refaktoryzacja kodu

Question

Witam, czy poniższy kod można napisać lepiej? Co można w nim poprawić?

Chciałem zapisać funkcję setFrom z klasy NameOfPlayer za pomocą bloków try-throw-catch, ale nie wiem jak zwrócić wynik do pozostałych klas, aby nie musiały obsługiwać wyjątków blokiem catch.

https://github.com/huberti1/NameOfPlayer

#pragma once

#include <fstream>
#include <string>

namespace ttt {
	class NameOfPlayer
	{
	public:
		bool setFrom(std::ifstream& file);

		void setFromInputStream();

		void saveTo(std::ofstream& file) const;

		std::string get() const;

	private:
		std::string name;

		bool tryToSetFrom(std::ifstream& file);

		std::string& getFromInputStream(std::string& name) const;

		bool hasNotProperSize(const std::string& name) const;

		bool isTooBig(const std::string &name) const;

		bool isTooSmall(const std::string &name) const;
	};

	inline std::string NameOfPlayer::get() const
	{
		return name;
	}
}

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

namespace ttt {
	bool NameOfPlayer::setFrom(std::ifstream& file)
	{
		return tryToSetFrom(file);
	}

	bool NameOfPlayer::tryToSetFrom(std::ifstream& file)
	{
		return getline(file, name) && file.get();
	}

	void NameOfPlayer::setFromInputStream()
	{
		name = getFromInputStream(name);
	}

	std::string& NameOfPlayer::getFromInputStream(std::string& name) const
	{
		std::cout << "Podaj swoje imie: ";
		getline(std::cin, name);
		while (hasNotProperSize(name))
		{
			if (isTooBig(name))
			{
				std::cout << "Imie moze miec maksymalnie 12 znakow. Sprobuj jeszcze raz:";
				getline(std::cin, name);
			}
			else if (isTooSmall(name))
			{
				std::cout << "Imie musi miec przynajmniej 3 znaki. Sprobuj jeszcze raz:";
				getline(std::cin, name);
			}
		}
		return name;
	}

	bool NameOfPlayer::hasNotProperSize(const std::string& name) const
	{
		return isTooBig(name) || isTooSmall(name);
	}

	bool NameOfPlayer::isTooBig(const std::string& name) const
	{
		return name.size() > 12;
	}

	bool NameOfPlayer::isTooSmall(const std::string& name) const
	{
		return name.size() < 3;
	}

	void NameOfPlayer::saveTo(std::ofstream& file) const
	{
		file << name << std::endl;
	}
}

#pragma once

#include "NameOfPlayer.h"
#include <string>
#include <fstream>

namespace ttt {
	class NameOfOnePlayer
	{
	public:
		NameOfOnePlayer();

		void setFromInputStream();

		std::string get() const;

	private:
		enum Exception { CANNOT_OPEN };
		const std::string nameOfFile = "nameOfOnePlayer.txt";

		NameOfPlayer nameOfPlayer;

		void setFromFile();

		void tryToSetFromFile();

		void saveToFile() const;

		void tryToSaveToFile() const;
	};

	inline std::string NameOfOnePlayer::get() const
	{
		return nameOfPlayer.get();
	}
}

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

namespace ttt {	
	NameOfOnePlayer::NameOfOnePlayer()
	{		
		setFromFile();
	}

	void NameOfOnePlayer::setFromFile()
	{
		try {
			tryToSetFromFile();
		}
		catch (const Exception& e) {
			if (e == CANNOT_OPEN)
			{
				setFromInputStream();
				saveToFile();
			}				
		}
	}

	void NameOfOnePlayer::tryToSetFromFile()
	{
		std::ifstream file;
		file.open(nameOfFile);
		if (!file.is_open())
		{
			file.close();
			throw CANNOT_OPEN;
		}
		else if (!nameOfPlayer.setFrom(file))
		{
			nameOfPlayer.setFromInputStream();
			file.close();
		}
		else
			file.close();
	}

	void NameOfOnePlayer::setFromInputStream()
	{
		nameOfPlayer.setFromInputStream();
	}

	void NameOfOnePlayer::saveToFile() const
	{
		try {
			tryToSaveToFile();
		}
		catch (const Exception& e) {
			if (e == CANNOT_OPEN)
				;//TODO log it to the file with communicates of errors
		}
		
	}

	void NameOfOnePlayer::tryToSaveToFile() const
	{
		std::ofstream file;
		file.open(nameOfFile);
		if (file.is_open())
		{
			nameOfPlayer.saveTo(file);
			file.close();
		}		
		else
		{
			file.close();
			throw CANNOT_OPEN;
		}		
	}
}

#pragma once

#include "NameOfPlayer.h"
#include <string>

namespace ttt {
	class NamesOfTwoPlayers
	{
	public:
		NamesOfTwoPlayers();

		void setFromInputStream();

		std::string getFirst();

		std::string getSecond();

	private:
		enum Exception { CANNOT_OPEN };
		const std::string nameOfFile = "namesOfTwoPlayers.txt";

		NameOfPlayer nameOfPlayer[2];

		void setFromFile();

		void tryToSetFromFile();

		void saveToFile();

		void tryToSaveToFile();
	};

	inline std::string NamesOfTwoPlayers::getFirst()
	{
		return nameOfPlayer[0].get();
	}

	inline std::string NamesOfTwoPlayers::getSecond()
	{
		return nameOfPlayer[1].get();
	}
}

#include "NamesOfTwoPlayers.h"
#include <iostream>
#include <fstream>

namespace ttt {
	NamesOfTwoPlayers::NamesOfTwoPlayers()
	{
		setFromFile();
	}

	void NamesOfTwoPlayers::setFromFile()
	{
		try {
			tryToSetFromFile();
		}
		catch (const Exception& e) {
			if (e == CANNOT_OPEN)
			{
				setFromInputStream();
				saveToFile();
			}				
		}
	}

	void NamesOfTwoPlayers::tryToSetFromFile()
	{
		std::ifstream file;
		file.open(nameOfFile);
		if (!file.is_open())
		{
			file.close();
			throw CANNOT_OPEN;
		}
		else
		{
			if (!nameOfPlayer[0].setFrom(file))
			{
				std::cout << "Imie pierwszego gracza." << std::endl;
				nameOfPlayer[0].setFromInputStream();
			}			
			if (!nameOfPlayer[1].setFrom(file))
			{
				std::cout << "Imie drugiego gracza." << std::endl;
				nameOfPlayer[1].setFromInputStream();
			}
			file.close();
		}		
	}

	void NamesOfTwoPlayers::setFromInputStream()
	{
		std::cout << "Imie pierwszego gracza." << std::endl;
		nameOfPlayer[0].setFromInputStream();

		std::cout << std::endl << "Imie drugiego gracza." << std::endl;
		nameOfPlayer[1].setFromInputStream();
	}

	void NamesOfTwoPlayers::saveToFile()
	{
		try {
			tryToSaveToFile();
		}
		catch (const Exception& e) {
			if (e == CANNOT_OPEN)
				;//TODO log it to the file with communicates of errors
		}
	}

	void NamesOfTwoPlayers::tryToSaveToFile()
	{
		std::ofstream file;
		file.open(nameOfFile);
		if (!file.is_open())
		{
			file.close();
			throw CANNOT_OPEN;
		}
		else
		{
			nameOfPlayer[0].saveTo(file);
			nameOfPlayer[1].saveTo(file);
			file.close();
		}
	}
}

 

Answer 1

Ten kod jest bardzo brzydki i nieczytelny :(

1) Po samej strukturze i nazwach klas widać, że albo ten kod nie ma sensu, albo nie przyłożyłeś się do nazewnictwa. NameOfPlayer i NameOfOnePlayer. Oceniając po nazwie te dwie klasy są tym samym. Po co więc je dzielić? Jak zajrzałem do środka to zobaczyłem, że NameOfOnePlayer to taki wraper na poprzednią klase, który dodaje jedynie funkcjonalność otwierania i zamykania pliku. Jest to po prostu źle zaprojektowane. Po pierwsze rozdzielenie tego to przerost formy nad treścią. Za bardzo się starasz robić to ładnie obiektowo przez co wychodzi z tego po prostu nieczytelny kod. Niepotrzebnie komplikowany. Generalnie powinieneś zrobić jakiś PlayerNameFileLoader, który dostanie nazwe pliku i będzie przez jedną metodę zwracał name. Aczkolwiek jeśli cała "logika" wczytania z pliku poza jego otwarciem i zamknieciem opiera się na:

getline(file, name);

To taka klasa nie za bardzo ma nawet sens. Gdyby ten plik miał jakiś format. Chociażby "w pierwszej lini ilosc imion, w kolejnych coś tam coś tam". Wtedy byłby sens stworzyć taką klase, która na podstawie znajomości tego formatu potrafi zapisywać i odczytywać nazwiska z pliku.

Co ciekawsze klasa NameOfPlayer potrafi się sama wczytać z konsoli, ale logika wczytywania z pliku jest już w klasie drugiej.

NameOfTwoPlayers. Co dalej? NameOfThreePlayers z identycznym ciałem poza tymi linijkami?

			nameOfPlayer[0].saveTo(file);
			nameOfPlayer[1].saveTo(file);
            nameOfPlayer[2].saveTo(file);

2) Cała zabawa z file.close() nie ma sensu, bo pliki same są zamykane w destruktorze (wywołanym po rzuceniu wyjątku) w stylu RAII:

destructs the basic_ifstream and the associated buffer, closes the file

 3) 

		if (!file.is_open())
		{
			file.close();
			throw CANNOT_OPEN;
		}
		else if (!nameOfPlayer.setFrom(file))

Jeśli chcesz rzucić wyjątek w ifie to nie potrzebujesz else:

if(somethingBadHappened)
    throw exception("oh no!");
//code if everythings fine

4) Testowałeś to wgl?
 

				std::cout << "Imie moze miec maksymalnie 12 znakow. Sprobuj jeszcze raz:";

bez przejscia do nowej lini.

5) Po co to file.get()

return getline(file, name) && file.get();

6) 

	void NameOfPlayer::setFromInputStream()
	{
		name = getFromInputStream(name);
	}

Te wszystkie wywołania w tym stylu nie mają sensu. Co wnosi funkcja, która ma jedną linijke i wywołuje inną funkcję z tej samej klasy?

7) 

std::string& NameOfPlayer::getFromInputStream(std::string& name) const

Bardzo zła nazwa funkcji. Brzmi jakby wczytywała z input streama a wczytuje ze standardowego wejścia (z konsoli). Input stream sugeruje różnego typu input streamy. Np istringstream czy ifstream. Obydwa sa input streamami.

8) 

namespace ttt {

bez komentarza :D

9) 

		enum Exception { CANNOT_OPEN };

jesli używasz enumów to używaj scoped enumów i nazywaj je sensowniej. Nazwałeś go Exception, co różni się od exception tylko jedną literką, więc ktoś może pomylić, szczególnie jeśli programuje też w innych językach gdzie akurat klasy standardowe są z dużych liter. Nazwij tego enuma zgodnie z przeznaczeniem. Już zwykłe FailureType by było lepsze niż exception.

10)

		const std::string nameOfFile = "nameOfOnePlayer.txt";

Cały sens tej klasy polega na wcztywaniu z pliku i nazwa pliku jest zahardkodowana w ciele? W tym momencie tworze sobie 2 obiekty tego typu i będą one sobie przeszkadzać, bo będą pisać do tego samego pliku.

Generalnie kod jest zły. Starasz sie pisać obiektowo i robić dużo metod, ale niestety niepotrzebnie gmatwasz proste rzeczy.

Answer 2

1. Zamiast :

#pragma once

Zobacz sobie: strażnik nagłówka.

2. Masz funkcje publiczną tylko po to , żeby wywoływać funkcje prywatną, trochę to dziwne.
3. Magic number`y, zwykle robisz się jakiegoś enum`a i tam się przypisuje co to coś oznacza, np. 2plik 47linijka "12" Co to jest to 12 ;o? Tzn. ja rozumiem, że wynika to z kontekstu, ale tak się po prostu robi :).
4. Namespace by mógł być trochę inny niż "ttt" :D
5. Mógłbyś zrobić tak, że w miejscu gdzie otwierasz plik ustawiać zmienną boolowską 

m_bWasFileOpen = true

I w destruktorze sprawdzać:

if( true == m_bWasFileOpen )
{
    file.close();
}

Oczywiście pamiętając o tym, że tam gdzie ten plik zamykasz, ustawiasz:

m_bWasFileOpen = false;

6.  Pomyślałes np. o tym, żeby zrobić sobie odzielnego enuma jakiegoś globalnego( czyt. oddzielny plik ), w którym będą kody błędów. Z każdym razem kiedy jest możliwość, że inna klasa wywoła funkcje innej klasy, czyli tak jak Ty masz, np.

nameOfPlayer.saveTo(file);

Niech funkcja saveTo zwraca kod błędu. Na przykład 0 kiedy wszystko jest OK a w innych przypadkach co innego. 
7. Odwołując się do funkcji w punkcie 6 "saveTo(file)" nie masz sprawdzania czy plik był już otwarty czy nie :).
8. Twoje pytanie: myślę, że jeżeli zrobił byś sobie taką zmienną boolowską, odnośnie czy plik jest otwarty czy też nie, to mogłoby to rozwiązać Twój problem.
9. Plik nagłówkowy w c++ to *.hpp a nie *.h
10. 

NamesOfTwoPlayers::getFirst()

Możesz zrobić jedną funkcje, która przyjmuje uint8_t ten argument byłby odpowiedzialny, za to, którego "playera" wybierasz.

Pokrótce to w sumie tyle, można by było dorzucić jeszcze privatne konstruktory kopiujące i operatory przypisania, jakąś oddzielną klasę tylko i wyłącznie do logowania, zmienne const, które inicjalizujesz w pliku *.h dać na liste inicjalizacyjną, zrobić tak, aby tylko klasa NameOfPlayer była odpowiedzialna za robienie ile graczy chcesz, a nie jak masz teraz oddzielna klasa, żeby stworzyć drugiego gracza.