I'm writing a simple game with multiplayer mode in C++, using the SFML framework.
Since I don't want to make it more complicated than necessary, I handle everything on a single thread, so that when the client is waiting for the remote player's turn, I check if there is an incoming message about the remote player's move. I do this in the following way, inside OnlineGame
class.
void OnlineGame::PlayGame(int& scoreOfPlayer1, int& scoreOfPlayer2) {
socket.setBlocking(false);
WaitForStartSignal();
map.Show(gameWindow);
currentPlayer->YourTurn();
while(!gameEnd) {
RunOneGameCycle();
}
SetPlayersScores(scoreOfPlayer1,scoreOfPlayer2);
}
void OnlineGame::RunOneGameCycle(){
gameWindow.GetInput();
GetRemoteMove();
if(turnEnd){
currentPlayer->YourTurn(); //the notification can only take place after the previous move was entirely completed,
//so that the current move will only be handled by one player, who is finishing their turn
turnEnd = false;
}
}
I wanted to refactor the GetInput
method of GameWindow
class, this method is the one responsible for getting input until some specific event happens, at which point the method should return.
void GameWindow::GetInput() {
Event event{};
//clearing event queue
while (pollEvent(event));
while (true) {
while (pollEvent(event)) {
if (active && event.type == Event::KeyPressed){
switch (event.key.code) {
case Keyboard::Up:
NotifyOnDirectionSelected(up);
return;
case Keyboard::Down:
NotifyOnDirectionSelected(down);
return;
case Keyboard::Right:
NotifyOnDirectionSelected(right);
return;
case Keyboard::Left:
NotifyOnDirectionSelected(left);
return;
case Keyboard::Escape:
NotifyOnExit();
return;
case Keyboard::Return:
NotifyOnConfirmation(true);
return;
default:
NotifyOnConfirmation(false);
return;
}
}
else if (event.type == Event::Closed) {
NotifyOnExit();
close();
return;
} else if (!active)
return;
}
}
}
It was obviously too long so I wanted to separate everything inside of the second while loop in a different method. Thus I refactored it in the following way:
void GameWindow::GetInput() {
ClearEventQueue();
Event event{};
inputEnd = false;
while (!inputEnd) {
while (!inputEnd && pollEvent(event)) {
HandleEvent(event);
}
}
}
void GameWindow::ClearEventQueue() {
Event event{};
while (pollEvent(event));
}
void GameWindow::HandleEvent(const Event& event) {
if(active && event.type == Event::KeyPressed){
HandleKeyPress(event.key.code);
inputEnd = true;
} else if(event.type == Event::Closed){
NotifyOnExit();
close();
inputEnd = true;
} else if (!active){
inputEnd = true;
}
}
void GameWindow::HandleKeyPress(const Keyboard::Key& key) {
switch (key) {
case Keyboard::Up:
NotifyOnDirectionSelected(up);
break;
case Keyboard::Down:
NotifyOnDirectionSelected(down);
break;
case Keyboard::Right:
NotifyOnDirectionSelected(right);
break;
case Keyboard::Left:
NotifyOnDirectionSelected(left);
break;
case Keyboard::Escape:
NotifyOnExit();
break;
case Keyboard::Return:
NotifyOnConfirmation(true);
break;
default:
NotifyOnConfirmation(false);
break;
}
}
inputEnd
is a member variable, and it is used to substitute the return
statements in the previous solution. active
denotes whether the window should wait for the local player's interaction (it is true
when it's the local player's turn). I think that this second version looks much more cleaner and is much more readable than the first one, but I still find the double while
loop with a partly same (similar looking) condition somewhat strange.
The declarations of these classes (with all the relevant methods):
GameWindow.h
using sf::RenderWindow;
using sf::Keyboard;
using sf::Event;
class GameWindow : public RenderWindow {
public:
void GetInput();
/**
* @brief Notifies the subscribed observers after the window containing the map has been closed.
*/
void NotifyOnExit() const;
/**
* @brief Notifies the subscribed observers after the user has selected a direction.
*/
void NotifyOnDirectionSelected(Direction) const;
/**
* @brief Notifies the subscribed observers whether the user has confirmed the action at hand.
*/
void NotifyOnConfirmation(bool) const;
private:
bool active; ///< Should the window create events?
bool inputEnd;
void ClearEventQueue();
void HandleKeyPress(const Keyboard::Key&);
void HandleEvent(const Event&);
};
OnlineGame.h
class OnlineGame : public Game {
public:
void PlayGame(int& scoreOfPlayer1, int& scoreOfPlayer2) override;
private:
/**
* @brief Waits for the remote player's move and notifies the Player instance that
* represents the remote player.
*/
void GetRemoteMove();
void WaitForStartSignal();
void RunOneGameCycle();
void SetPlayersScores(int& scoreOfPlayer1, int& scoreOfPlayer2);
};
Is there a way to make it even cleaner, and readable while retaining the same functionality? (it's important to note that the second while
should stop right after some specific events happen, that's why I had to check the inputEnd
there as well) Also, did I otherwise do a good job of refactoring the function, is there any modification I should make here?