Keeping track of the tennis score

Question

I'm trying to get hired by some company and they gave me a simple remote test to take at home so they can judge my coding skills.

It's a really simple problem based on the website HackerRank. It's a mini game where you have to keep track of the score of a tennis game (0-15-30-40-ADV).

This is really basic and I managed to do it without any problems. But I believe the more important thing is not if it works or not but rather how you did it and if you did code like a professional.

What could I improve on in my code to actually code like a professional?

//======================================
// Author: Philippe Balleydier
// Last Update : 08/09/16
// Object : Little algorithm able to keep track of the tennis score
//======================================


#include <iostream>
#include <stdlib.h>
#include <time.h>
#include <string>

using namespace std;

// score map for printing the score
const string Score[] = {"0", "15", "30", "40", "ADV"};

//======================================
// Class Game: contains all the informations and methods about the game being played
//======================================
class Game
{
    // see below for explanations
    public:
    Game();
    bool createPlayer(int playerId, string playerName);
    string getName(int playerId);
    bool hasADV(int playerId);
    bool setScore(int playerId, int score);
    int getIntScore(int playerId);
    string getStringScore(int playerId);

    private:
    string players[2]; // players' name
    int scores[2]; // players' score
};


//--------------------------------------
// Constructor : init score to 0
//--------------------------------------
Game::Game()
{
    scores[0] = 0;
    scores[1] = 0;
}

//--------------------------------------
// createPlayer : create a player depending on its ID
//--------------------------------------
bool Game::createPlayer(int playerId, string playerName)
{
    if (playerId == 0 || playerId == 1)
    {
        players[playerId] = playerName;
        return true;
    }
    else
        return false;
}

//--------------------------------------
// getName : return the name of player "playerId" as a string
//--------------------------------------
string Game::getName(int playerId)
{
    if (playerId == 0 || playerId == 1)
        return players[playerId];
    else
        return "N/A";
}

//--------------------------------------
// getName : return true if the player's score is ADV, else false
//--------------------------------------
bool Game::hasADV(int playerId)
{
    if (scores[playerId]==4)
        return true;
    else
        return false;
}

//--------------------------------------
// setScore : change the score of player "playerId"
//--------------------------------------
bool Game::setScore(int playerId, int score)
{
    if (score < 5)
        scores[playerId]=score;
    else
        return false;
}

//--------------------------------------
// getIntScore : return the score of player "playerId" as a int (0, 1, 2, 3, 4)
//--------------------------------------
int Game::getIntScore(int playerId)
{
    return scores[playerId];
}

//--------------------------------------
// getStringScore : return the score of player "playerId" as a string (0, 15, 30, 40, ADV)
//--------------------------------------
string Game::getStringScore(int playerId)
{
    return Score[scores[playerId]];
}
//=======================================

int main()
{
    // rand init
    srand(time(NULL));

    // setting up the game
    Game myGame;
    myGame.createPlayer(0, "John");
    myGame.createPlayer(1, "Paul");

    //starting game loop
    bool stop = false;
    while(!stop)
    {
        // identifying winner and looser
        int pointWinner = rand()%2;
        int pointLooser=(pointWinner+1)%2;
        cout << "The winner of the point is " << myGame.getName(pointWinner) << endl;

        // The pointWinner has advantage => win
        if (myGame.hasADV(pointWinner))
        {
            stop = true;
            cout << "Winner is " << myGame.getName(pointWinner) << endl;
        } // The opponent has advantage => tie (40-40)
        else if (myGame.hasADV(pointLooser))
            myGame.setScore(pointLooser, 3);
        // Tie (40-40) => pointWinner gets advantage
        else if (myGame.getIntScore(pointWinner)==3 && myGame.getIntScore(pointLooser) == 3)
            myGame.setScore(pointWinner, 4);
        //pointWinner is 1 point ahead of pointLooser and >=30
        else if (myGame.getIntScore(pointWinner)>myGame.getIntScore(pointLooser) && myGame.getIntScore(pointWinner)>=2)
        {
            stop = true;
            cout << "Winner is " << myGame.getName(pointWinner) << endl;
        }
        else // nothing special, pointWinner marks the point
            myGame.setScore(pointWinner, myGame.getIntScore(pointWinner)+1);

        // printing score
        if (!stop)
            cout << "Score : " << myGame.getName(0) << " " << myGame.getStringScore(0) << " - " << myGame.getStringScore(1) << " " << myGame.getName(1) << endl;
    }

    return 0;
}

Just so you know, it has to be a 1-page .cpp file, so I can't really split the Game class into different .h and .cpp files.

Answer 1

Overall

You missed the point.
If you are supposed to create a class to track the score of game you did it wrong. Most of the logic that should be inside the class has been left inside main.

If you see the pattern:

1. Get info from object.
2. Manipulate data.
3. Update state of object.

This usually means that you should have a function called Manipulate() (where Manipulate is the action you are doing).

I would expect the logic running the game to be:

while(game)
{
     game.winPoint(rand() % 2);
     std::cout << game << "\n";
}
std::cout << "Winner was: " << game.winner() << "\n";

Thus your interface to the class should be:

class Game
{
    public:
        // returns true if the game is still being played
        bool operator() const;

        // A player won a point (updates the state.
        void winPoint(int playerId);

        // return a reference to the winning player.
        // If the game is not finished calling this is UB.
        Player& winner() const;

        // Print the current state of the game.
        void print(std::ostream& str) const;

        friend std::ostream& operator<<(std::ostream& s, Game const& g)
        {
            g.print(s);
            return s;
        }
};

Code Review

Comments.

Comments should be meaningful.

A bad comment is worse than no comment. This is because like code, comments need to be maintained. If the comments and code fall out of sync then what does a maintainer do? Fix the code or fix the comment?

So comments should be for describing why (not how).

The first two lines are OK.

//======================================
// Author: Philippe Balleydier
// Last Update : 08/09/16
// Object : Little algorithm able to keep track of the tennis score
//======================================

The trouble is the third line is not accurate. It's not an algorithm it's a class.

This comment on the other hand is completely useless. It tells me nothing that I don't already know.

//======================================
// Class Game: contains all the informations and methods about the game being played
//======================================

Header files.

These are C header files.

#include <stdlib.h>
#include <time.h>

There are C++ versions of these header files. The C++ versions put all the appropriate declarations in the namespace std.

#include <cstdlib>
#include <ctime>

Never Do this.

using namespace std;

If you had read any other C++ review on this board it would have told you not to do this. See: Why is “using namespace std” in C++ considered bad practice?.

There is a reason that the standard libraries are called std and not standard. Its to make prefixing them easy.

This should be a private member of the class.

// score map for printing the score
const string Score[] = {"0", "15", "30", "40", "ADV"};

Better indenting

Nice indenting is critical to coding. The compiler does not care. But the code will have to be read by humans long after you abandon this code to some poor maintainer. So please make it easy to read.

    class Game
    {
        // see below for explanations
        public:
   =>   Game();
   =>   bool createPlayer(int playerId, string playerName);

I want to see that those methods are inside the public region of the class.

Constructor

Is that comment telling me something I can not read in the code. Remove useless comment.

//--------------------------------------
// Constructor : init score to 0
//--------------------------------------
Game::Game()
{
    scores[0] = 0;
    scores[1] = 0;
}

The problem is that you have not set up the class completely. You have just created an empty object. You require the user of your class to make two further calls to set up the players before the class is ready to use. This means I can accidentally start using the class before it is completely set up.

I would have done it like this:

 Player player1("Loki");
 Player player2("Thor");
 Game   game(player1, player2);

 // Game is now in a valid state and ready to go.

Use exceptions when you have an error

bool Game::createPlayer(int playerId, string playerName)

OK so I fail to add a player now what. The fact that you have an error condition probably means that there is a serious flaw in your code (the code that is calling the class) so your application should probably fail fast rather than try to continue.

if (!myGame.createPlayer(2, "Bob"))
{
    // Failed to create player.
    // what can I do here?
}

Also you return an error code but fail to even try and check it in your main application. That is a serious flaw (especially in interview code).

myGame.createPlayer(0, "John");  // returns an unchecked bool !!!!!
myGame.createPlayer(1, "Paul");

Getter/Setter Bad practice

It means you are leaking implementation details of the code. Prefer to use action methods that internally mutate the state of the object.

Note: Getter/Setter are fine on object bags; but not on classes.

Aside: Another bad comment (on each function). The comment is unneeded because the function is well named, so well that I can see exactly what it is doing without the comment.

//--------------------------------------
// getName : return the name of player "playerId" as a string
//--------------------------------------
string Game::getName(int playerId)    
bool Game::hasADV(int playerId)
bool Game::setScore(int playerId, int score)    
int Game::getIntScore(int playerId)
string Game::getStringScore(int playerId)

Use the new random number

    // rand init
    srand(time(NULL));

Sure it works. For this kind of example it is also fine. But this is an interview question. You want to show that you are up-to-date with modern C++ so use the new random number stuff.

Main

main() is special. If you don't return a value from main then the compiler will plant return 0; for you. So don't add a return to main.

I use the fact that if there is a return 0; in my code that it indicates that at another point in main there is a return that is not zero.

Answer 2

@Loki Astari is completely right in his points. Apart from that you should consolidate your logic, as currently you have 5 clauses for 3 cases, which are:

1. pointWinner wins the game
2. pointLoser had advantage
3. Everything else

So you have overcomplicated the code, which is also true for those setters and getters.

---

Think about what is necessary and what are the correct types. Your score is an int. Is there any meaning of an negative score? If no use unsigned.

---

You should put things together that belong together. A player has a name and a score and a game has two payers.

#include <iostream>
#include <string>
#include <utility>
#include <vector>
class Game {
    using player = std::pair<std::string, unsigned>;
private:
    std::vector<player> players;
}

Now how do we get a game? We need two players identified by their names which start both at 0. Note that we do not allow any other constructor via explicit keyword and seed the RNG.

#include <iostream>
#include <string>
#include <utility>
#include <vector>
class Game {
    using player = std::pair<std::string, unsigned>;
public:
    explicit Game(const std::string &name1, const std::string &name2)
    : players({player(name1, 0), player(name2, 0)}) {srand(time(NULL));}
private:
    std::vector<player> players;
}

Next think about what is happening during the game. The players are playing a point, the game and we are telling the score, which gives us the following public functions:

void playGame (void)
void playPoint (void)
void printScore (void)

So let's implement the functions (for simplicity I do not use a .cpp file which you should most likely).

#include <iostream>
#include <string>
#include <utility>
#include <vector>
class Game {
    using player = std::pair<std::string, unsigned>;
public:
    explicit Game(const std::string &name1, const std::string &name2)
    : players({player(name1, 0), player(name2, 0)}) {srand(time(NULL));}

    void printScore (unsigned pointWinner) {
        if (gameWon == -1) {
        std::cout << players.at(pointWinner).first << " made a point. "
                  << "The current score is: \n";
        std::cout << players.at(0).first << ":\t"
                  << Scores.at(players.at(0).second) << std::endl;
        std::cout << players.at(1).first << ":\t"
                  << Scores.at(players.at(1).second) << std::endl;
        } else {
            std::cout << players.at(pointWinner).first << " won the game.\n";
        }
    }

    void playGame (void) {
        while (gameWon == -1) {
            playPoint();
        }
    }

    void playPoint (void) {
        unsigned pointWinner = rand()%2;
        unsigned pointLoser  = (pointWinner+1)%2;

        if (players.at(pointWinner).second == 4 ||
            (players.at(pointWinner).second == 3 &&
             players.at(pointLoser).second < 3)) {
            gameWon = pointWinner;
        } else if (players.at(pointLoser).second == 4) {
            players.at(pointLoser).second--;
        } else {
            players.at(pointWinner).second++;
        }
            printScore(pointWinner);
    }

private:
    std::vector<player> players;
    int gameWon = -1;
    const std::vector<std::string> Scores = {"0", "15", "30", "45", "Adv"};
};

---

So we play a point. Flip a coin and check if the game is won. We print the score depending whether the game has ended or not. We play the game as long as it is not ended. Now we have to think about what we need to make publicly available.

printScore is only used in playPoint, which in turn is only used in playGame, so these two can be private functions.

class Game {
    using player = std::pair<std::string, unsigned>;
public:
    explicit Game(const std::string &name1, const std::string &name2)
    : players({player(name1, 0), player(name2, 0)}) {srand(time(NULL));}

    void playGame (void) {
        while (gameWon == -1) {
            playPoint();
        }
    }

private:
    std::vector<player> players;
    int gameWon = -1;
    const std::vector<std::string> Scores = {"0", "15", "30", "45", "Adv"};

    void playPoint (void) {
        unsigned pointWinner = rand()%2;
        unsigned pointLoser  = (pointWinner+1)%2;

        if (players.at(pointWinner).second == 4 ||
            (players.at(pointWinner).second == 3 &&
             players.at(pointLoser).second < 3)) {
            gameWon = pointWinner;
        } else if (players.at(pointLoser).second == 4) {
            players.at(pointLoser).second--;
        } else {
            players.at(pointWinner).second++;
        }
            printScore(pointWinner);
    }

    void printScore (unsigned pointWinner) {
        if (gameWon == -1) {
        std::cout << players.at(pointWinner).first << " made a point. "
                  << "The current score is: \n";
        std::cout << players.at(0).first << ":\t"
                  << Scores.at(players.at(0).second) << std::endl;
        std::cout << players.at(1).first << ":\t"
                  << Scores.at(players.at(1).second) << std::endl;
        } else {
            std::cout << players.at(pointWinner).first << " won the game.\n";
        }
    }
};

And your main is simply:

#include "Tennis.h"
int main()
{
    Game USOpen= Game("Loki", "Thor");
    USOpen.playGame();
}

Answer 3

#include <iostream>
#include <stdlib.h>
#include <time.h>
#include <string>

Organize your header files. Larger programs will have a longer list of includes. Maintainers of your code will want to be able to trace your list of includes efficiently, and there is no faster way than binary search on a sorted list.

Prefer the C++ Standard Library headers. Use of the C Standard Library headers is deprecated by the standard and should be avoided with new codebases.

Note— Any reference to the C++ Standard document refers to the last public draft of the C++14 standard (n4140). Drafts can be found on github. —End Note

D.5 C standard library headers [depr.c.headers]

\$^1\$ For compatibility with the C standard library and the C Unicode TR, the C++ standard library provides the 26 C headers, as shown in Table 155.

\$^2\$ Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (§3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (§7.3.3).

\$^3\$ [ Example: The header <cstdlib> assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header <stdlib.h> assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. —end example ]

---

using namespace std;

using directives can be useful when used correctly (namespace composition). Importing all symbols into the global namespace to avoid namespace qualification is not one of them. Associated side effects include collisions and ADL-related issues.

---

const string Score[] = {"0", "15", "30", "40", "ADV"};

Use enumerations to represent sets of related constants allowing you to separate logic from I/O (not all tennis scoring system use 0-15-30-40, some simply use 1-2-3-4).

Avoid global variables. Score is only depended on by a single entity (Game). Make Score a data member of Game.

---

Game::Game() {
    scores[0] = 0;
    scores[1] = 0;
}

A default constructor should do more than just initialize members variables with constants. Prefer to use in-class data member initializers and use the compiler-generated default constructor.

class Game {
    std::string players[2]{};
    int scores[2]{};
  public:
    // Use compiler-generated default constructor
    Game() = default;
    // ...
};

---

bool Game::createPlayer(int playerId, string playerName) {
    if (playerId == 0 || playerId == 1) {
        players[playerId] = playerName;
        return true;
    }
    else
        return false;
}

Prefer to acquire resources and establish all invariants on construction (see RAII aka SBRM, CADRe). With multi-stage initialization, every method needs to address the problem "what do you do when the object is in the uninitialized state?". To handle this, you provide more conditionals which make your functions heavier.

Your program doesn't handle the uninitialized players[]. Reading from the array is undefined behavior.

Prefer std::size_t instead of int or unsigned int for indices. From the C++ Standard:

18.2 Types [support.types]

\$^6\$ The type size_t is an implementation-defined unsigned integer type that is large enough to contain the size in bytes of any object.

---

string Game::getName(int playerId) {
    if (playerId == 0 || playerId == 1)
        return players[playerId];
    else
        return "N/A";
}

Avoid else after early exits (return, break, continue, throw). You will reduce indentation level and amount of code that needs to be tracked when reading.

string Game::getName(int playerId) {
    if (playerId == 0 || playerId == 1) {
        return players[playerId];
    }
    return "N/A";
}

"N/A" could be the name entered by the user which is the same type/value as the error value. Notify the callee when errors occur through an exception mechanism. There are plenty of options that exist in the modern C++ world. Options include plain exceptions (<stdexcept>), error conditions (<system_error>), and union types (boost::optional, std::optional, Alexandrescu's Expected<T>).

    std::string Game::getName(int playerId) {
        range_check(playerId);
        return players[playerId];
    }
    // ...
private:
    void range_check(const std::size_t index) {
        if (index >= std::size(players)) {
            throw std::out_of_range("Invalid Index");
        }
    }

---

bool Game::setScore(int playerId, int score) {
    if (score < 5)
        scores[playerId]=score;
    else
        return false;
}

Avoid magic constants. Use constexpr named variables or enumerations to give the literals meaningful context. Some literals are acceptable (-1, 0, 1, nullptr, '\n').

Make sure you are compiling with warnings enabled (-Wall -Wextra). If you plan to write standard conforming code, enable pedantic warnings as well (-pedantic). GCC and Clang both report:

In member function 'bool Game::setScore(int, int)':
warning: control reaches end of non-void function [-Wreturn-type]

From the C++ Standard:

6.6.3 The return statement [stmt.return]

\$^2\$ ... Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function.

---

int Game::getIntScore(int playerId) {
    return scores[playerId];
}

Your other functions performed a range check. What about getIntScore and getStringScore?

---

srand(time(NULL));
// ...
int pointWinner = rand()%2;

rand() is considered harmful. Prefer to use C++ standard <random> library, boost::random, or the PCG Random library.

---

bool stop = false;
while(!stop) {
   // ...
    if (myGame.hasADV(pointWinner)) {
        stop = true;
        cout << //...
    } // ...
}

Rather than having a variable to track the state, consider interrupting the control flow with a break.

while (true) {
    // ...
    if (myGame.hasADV(pointWinner)) {
        std::cout << //...
        break;
    } // ...
}

Keep functions short and simple, to the point that they perform a single logical operation.

---

else if (myGame.getIntScore(pointWinner)>myGame.getIntScore(pointLooser) && myGame.getIntScore(pointWinner)>=2)

Test! Test! Test! Your score keeper exits early. A sample output:

The winner of the point is John
Score : John 15 - 0 Paul
The winner of the point is John
Score : John 30 - 0 Paul
The winner of the point is John
Winner is John

John should hit match point (40) before winning.