跟踪网球比分_编程黑洞网

我正试图被某家公司聘用，他们给了我一个简单的远程测试，可以带回家进行学习，以便他们判断我的编码技能。

这是一个非常简单的问题，基于HackerRank网站。这是一款迷你游戏，您必须跟踪网球比赛的得分（0-15-30-40-ADV）。

这真的很基础，我设法做到了没有任何问题。但是我相信更重要的是，它是否有效，而不是您是如何做到的，以及您是否像专业人士一样编写代码。

我可以在代码中进行哪些改进以使实际代码像是专业的吗？

//======================================
// 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;
}

就知道，它必须是1页.cpp文件，所以我不能真正将Game类拆分为不同的.h文件。和.cpp文件。

尽管我有两三件事要说，交出面试代码的不诚实行为已经由经验丰富的程序员大大改善了，但是有人可以正确地反驳，说您可以略过这是面试过程的一部分，因此没有我们本来会更明智（包括您的潜在雇主）。因此，我将为此留给您-不要上交不属于您的工作。我并不是说您不能使用Google，但是如果您的公司看到您从此处提交的代码级别，

如果他们雇用您，那将是他们的期望。除非他们不会给您时间来编写代码，在此处发布代码并等待答案。此外，此处的用户可能并不总是倾向于为与无聊的工作相关的实际代码提供改进的解决方案；在这种情况下，您的代码是有趣的“游戏”代码。那么当老板看到的代码比面试上交的代码成熟几年时，您会怎么做？提及：“哦，我把它变成了代码？我首先得到了在线社区的直接帮助。”我的意思是，@ Loki进行了一些重大改进...所以要明智。

#1 楼

总体

您错过了重点。
如果您应该创建一个类来跟踪游戏的得分，那您做错了。应该包含在类中的大多数逻辑已留在main中。

如果看到模式：

从对象获取信息。
操作数据。
更新对象的状态。

这通常意味着您应该具有一个名为Manipulate()的函数（其中“操作”是您正在执行的操作）。

我希望运行游戏的逻辑是：

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

因此，您与该类的接口应为：

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;
        }
};

代码审查

评论。

评论应该有意义。

不好的评论比没有评论要糟。这是因为像代码一样，注释也需要维护。如果注释和代码不同步，那么维护者会做什么？修复代码或修复注释？

所以注释应该用于描述原因（而不是方法）。

前两行都可以。

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

问题是第三行不准确。这不是算法，而是类。

另一方面，此注释完全没有用。它告诉我我不知道的所有内容。

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

这些是C头文件。

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

这些头文件有C ++版本。 C ++版本将所有适当的声明放在名称空间std中。

#include <cstdlib>
#include <ctime>

切勿执行此操作。

using namespace std;

如果您在此板上阅读了任何其他C ++评论，它会告诉您不要这样做。请参阅：为什么在C ++中“使用命名空间std”被认为是不好的做法？。

有一个理由将标准库称为std而不是standard。它可以使它们的前缀变得容易。

这应该是该类的私有成员。

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

更好地缩进

好的缩进对于编码至关重要。编译器不在乎。但是，在您将代码遗弃给一些可怜的维护者之后，很长一段时间以来，人类都必须阅读该代码。因此，请使其易于阅读。

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

我希望看到这些方法在类的公共区域内。

构造函数

那条评论告诉我一些我在代码中看不到的东西。删除无用的评论。

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

问题是您尚未完全设置课程。您刚刚创建了一个空对象。在班级准备使用之前，您需要班级用户再打两次电话来设置播放器。这意味着在完全设置类之前，我可能会不小心开始使用它。

我会这样做：

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

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

使用发生错误时出现异常

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

好，所以我现在无法添加播放器。错误情况的事实可能意味着您的代码（调用类的代码）中存在严重缺陷，因此您的应用程序可能应该快速失败而不是尝试继续。

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

您还返回一个错误代码，但是甚至无法尝试在主应用程序中对其进行检查。这是一个严重的缺陷（尤其是在面试代码中）。

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

Getter / Setter错误的做法

，这意味着您在泄漏实现的细节编码。最好使用在内部改变对象状态的动作方法。

注意：吸气剂/塞特剂可以很好地用于对象包装袋。而不是在类上。

在旁边：另一个不好的注释（在每个函数上）。不需要注释，因为该函数的名称很好，因此不用注释就可以准确看到它在做什么。

//--------------------------------------
// 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)

使用新的随机数

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

当然可以。对于这种示例，也很好。但这是一个面试问题。您想证明自己是最新的C ++，所以请使用新的随机数。

Main

main()很特殊。如果您没有从main返回值，则编译器将为您植入return 0;。因此，请勿在main上添加收益。

我使用这样的事实：如果我的代码中存在return 0;，则表明在main的另一点上存在不为零的收益。 br />

\ $ \ begingroup \ $
“盖特/塞特可以装满物品袋”，是真的吗？在那种情况下，您肯定只需要直接访问内部对象的结构吗？
\ $ \ endgroup \ $
–cloakedlearning
16年9月8日在20:08

\ $ \ begingroup \ $
@cloakedlearning：是的，我更喜欢仅使它们成为开放结构。但是有时候您需要吸气剂/吸气剂（但是我很容易被认为不适合该职位）。您只想在课堂上避免使用它们。
\ $ \ endgroup \ $
–马丁·约克
16年8月8日在21:17

\ $ \ begingroup \ $
我同意应该删除Game :: Game（）之前的注释；实际上，以我的经验，在函数定义上方写注释很少是有意义的。但是最好在函数声明上方写一个注释，说明函数的功能。在较大的项目中，如果使注释适合于文档生成器，则这特别有价值，即使注释中的内容通过查看函数定义而显而易见。
\ $ \ endgroup \ $
– David K
16 Sep 8'在22:16

\ $ \ begingroup \ $
@DavidK文档生成将把函数定义也放进去。如果没有注释，但是代码是自我记录的，则效果相同。但是，任何复杂的内容都应带有注释（以及描述“什么”（而不是“如何”）的注释）。
\ $ \ endgroup \ $
–马丁·约克
16-9-09 at 17:41

\ $ \ begingroup \ $
@ Nik-Lz：评论：使用评论来说明您要实现的目标。使用“自我文档”代码来说明您如何执行此操作。解释代码的注释是没有用的（比我认为有害的无用更糟糕）。这是因为解释代码的注释需要与代码一起维护。不幸的是，这种情况很少发生，并且代码和注释不同步。因此，现在维护人员必须修复注释或修复代码，但是！自我记录代码应使解释该代码的注释变得多余（因此，对代码的工作原理无注释）。
\ $ \ endgroup \ $
–马丁·约克
18-10-19在12:55

#2 楼

@Loki Astari完全正确。除此之外，您应该巩固自己的逻辑，因为目前您有3种情况的5个子句，分别是：

pointWinner赢得了比赛

pointLoser具有优势
其他所有优点

所以您使代码过于复杂，对于那些设置者和获取者也是如此。

考虑什么是必需的，什么是正确的类型。你的分数是一个整数。负分数有什么意义吗？如果没有，请使用unsigned。

您应该把属于一起的东西放在一起。一个玩家有一个名字和一个分数，一个游戏有两个支付者。

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

现在我们如何获得游戏？我们需要两个以其名称开头且都从0开始的玩家。请注意，我们不允许通过显式关键字使用任何其他构造函数并为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;
}

下一个思考关于比赛中发生的事情。玩家正在玩一个点子，在游戏中，我们在讲分数，这为我们提供了以下公共功能：

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

因此，让我们实现这些功能（为简单起见，请不要使用您最可能使用的.cpp文件。）

#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"};
};

所以我们提出了一个要点。掷硬币并检查游戏是否获胜。我们根据游戏是否结束来打印分数。只要没有结束，我们就玩游戏。现在，我们必须考虑需要公开发布的内容。

printScore仅在playPoint中使用，而后者仅在playGame中使用，因此这两个可以是private函数。

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";
        }
    }
};

您的main很简单：

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

\ $ \ begingroup \ $
我不同意如果没有使用未签名。唯一真正使用无符号的时间是使用整数作为一组位标志时。麻烦的是，将一个未签名的接口传递给一个已签名的接口是因为它将被自动转换为一个未签名的值。之后就无法检测到（除此之外可能很大）。使用带符号的值更容易，它使您可以检查负值。
\ $ \ endgroup \ $
–马丁·约克
2016年9月8日14:39

\ $ \ begingroup \ $
我在代码中发现了一个错误，将GameWon与第一个玩家的0进行比较。应该是int gameWon = -1并对照该签名或未签名gameWon并对照<2
\ $ \ endgroup \ $
– miscco
16年9月8日在17:30

\ $ \ begingroup \ $
@LokiAstari我一般不会说，例如对于循环，通常最好使用unsigned / size_t。但是，评论更多是关于根据问题使用正确的类型。
\ $ \ endgroup \ $
– miscco
16年8月8日在17:38

\ $ \ begingroup \ $
为什么不使用枚举？分数不是一个数字，而是一小部分值。
\ $ \ endgroup \ $
– ruds
2016年9月9日下午0:31

\ $ \ begingroup \ $
是的，一个枚举原则上将提高可读性。实际上，最好的方法是map << enum，std :: string>，这样我们也可以使用它进行打印
\ $ \ endgroup \ $
– miscco
16年9月9日在7:13

#3 楼

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

组织头文件。较大的程序将包含更长的包含列表。您的代码维护者将希望能够高效地跟踪包含列表，并且没有比在排序列表上进行二进制搜索更快的方法。

首选C ++标准库标头。标准不建议使用C标准库标头，并且在新的代码库中应避免使用。

注：对C ++标准文档的任何引用均指C ++ 14标准（n4140）的最新公开草案。草稿可以在github上找到。 —尾注

D.5 C标准库头文件[depr.c.headers]

\ $ ^ 1 \ $与C标准库兼容和C Unicode TR，C ++标准库提供了26个C头，如表155所示。

\ $ ^ 2 \ $每个C头，每个头的名称形式均为name.h的行为就像是将由相应cname标头放置在标准库命名空间中的每个名称都放在全局命名空间范围内一样。未指定这些名称是先在名称空间std的名称空间范围（第3.3.6节）中声明还是定义，然后通过显式using-声明（第7.3.3节）注入到全局名称空间范围中。

\ $ ^ 3 \ $ [示例：标头<cstdlib>肯定在名称空间std中提供了其声明和定义。它还可以在全局名称空间中提供这些名称。头文件<stdlib.h>可以肯定地在全局名称空间中提供与C标准中相同的声明和定义。它还可以在名称空间std中提供这些名称。 —结束示例]

using namespace std;

正确使用using伪指令（命名空间组成）时可能很有用。将所有符号导入全局名称空间以避免名称空间限定不是其中之一。相关的副作用包括冲突和与ADL相关的问题。

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

使用枚举表示一组相关的常量，使您可以将逻辑与I / O分开（并非所有的网球计分系统都使用0-15-30-40，有些只使用1-2-3-4）。

避免使用全局变量。 Score仅由单个实体（Game）所依赖。使Score成为Game的数据成员。

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

默认构造函数应该做的不仅仅是用常量初始化成员变量。最好使用类内数据成员初始化程序，并使用编译器生成的默认构造函数。

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;
}

获取资源并建立构造的所有不变式（请参阅RAII，又称SBRM，CADRe）。使用多阶段初始化，每种方法都需要解决“对象处于未初始化状态时您将做什么？”的问题。为了解决这个问题，您提供了更多条件，使您的功能更重。

您的程序无法处理未初始化的players[]。从数组读取是未定义的行为。

首选std::size_t而不是int或unsigned int作为索引。来自C ++标准：

18.2类型[support.types]

\ $ ^ 6 \ $类型size_t是实现定义的无符号整数类型，它足够大以包含任何对象的字节大小。

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

提早退出后请避免使用else（return，break，continue， throw）。您将减少缩进级别和阅读时需要跟踪的代码量。

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

"N/A"可以是用户输入的名称，该名称/类型与错误值相同。通过异常机制在发生错误时通知被叫方。现代C ++世界中存在许多选择。选项包括普通异常（<stdexcept>），错误条件（<system_error>）和union类型（boost::optional，std::optional，Alexandrescu的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;
}

避免魔术常数。使用constexpr命名变量或枚举来赋予文字有意义的上下文。某些文字是可以接受的（-1，0，1，nullptr，'\n'）。

请确保您在启用警告的情况下进行编译（-Wall -Wextra）。如果您打算编写符合标准的代码，请同时启用pedantic警告（-pedantic）。 GCC和Clang均报告：

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

来自C ++标准：

6.6。 3 return语句[stmt.return]

\ $ ^ 2 \ $ ...从函数末尾流出就等于没有值的return；这会导致返回值的函数发生不确定的行为。

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

您的其他函数执行了范围检查。 getIntScore和getStringScore怎么办？

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

rand()被认为是有害的。最好使用C ++标准<random>库，boost::random或PCG随机库。

与其使用变量来跟踪状态，不如考虑中断状态。使用break进行控制流。

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

使函数简短易用，以至于它们执行单个逻辑运算。

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

测试！测试！测试！您的得分守门员会早退。输出示例：

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

John应该在赢得比赛之前达到比赛点（40）。