我隔离了一些代码,这引起了我和另一个用户之间的一场小辩论。我已经接受了他所说的一些内容,并将其与此处首先要审阅的代码相结合。 br />


我是根据我们的谈话提出的。

我是否需要创建一个新方法来整理用户输入以及它是否在最小和最大范围内?

评论

您将playerChoice初始化为-1,但从未使用过该值。

validInput = int.TryParse(Console.ReadLine(),out playerChoice); @CodesInChaos

该行会覆盖玩家选择的初始值,并且不会读取它(这是一个out参数),因此您无需将其初始化为-1。相反,我将其声明为int playerChoice;无需初始化。

除非您真的需要使用无类型的变量,否则强类型通常是更好的选择,因为它可以帮助捕获愚蠢的错误。这是针对错误的附加防护。

这是C#,没有未类型化的变量。所有变量都是强类型的。

#1 楼

第二个样本肯定更好。我倾向于通过将机制与策略分开来继续改进代码。策略是实际上表达程序含义的代码。该机制是表示哪些特定操作实施该策略的代码。

这个想法来自安全设计;您不希望计算出的代码“鲍勃被允许打开这扇门吗?”以及计算出的代码“真的是鲍勃的卡钥匙吗?”是相同的代码。第一个是政策,第二个是机制。但这通常适用于所有代码。

通过将机制与策略分开,我们使两者都更容易理解。您代码的“主线”应与规范类似。如果您必须用英语描述您的代码,则可能会说“向用户提供各种手势选择。如果用户选择了无效手势,请继续尝试直到他们选择了有效手势”。那就是程序的意思,但这不是代码的样子。相反,您的代码看起来好像世界上最重要的事情是整数和布尔变量,列表计数等。

让我们确定一种机制:解析整数并测试其是否在范围内。 “用户必须选择有效手势”策略背后的机制。因此,让我们将该机制隔离为一个纯粹的机械方法,该方法对您的策略域一无所知:

。看看我们删除了多少丑陋的机制,使您的政策更加耀眼:

static class Extensions
{
    public static int? BoundedParse(this string str, int lower, int upper)
    {
        if (str == null) 
            return null;
        int result;
        bool success;
        success = int.TryParse(str, out result);
        if (!success) 
            return null;
        if (result < lower) 
            return null;
        if (result > upper) 
            return null;
        return result;
    }
}


作为奖励,我们现在有一种可重用的方法,可以应用于任何任务这要求将字符串解析为有界整数。

我们可以更进一步。这不是一种循环机制吗?将其移到辅助方法中:

gameSetup(listOfGestures);
int? choice;
do
{
    Console.WriteLine("Please choose your Gesture ");
    choice = Console.ReadLine().BoundedParse(1, listofGestures.Count);
}
while(choice == null);
playerGesture = listOfGestures[choice.Value - 1];


现在您的方法是:

static int PromptForNumber(string prompt, int lower, int upper)
{
    int? choice;
    do
    {
        Console.WriteLine(prompt);
        choice = Console.ReadLine().BoundedParse(lower, upper);
    }
    while(choice == null);
    return choice.Value;
}


现在,代码非常清楚了策略级别上发生的事情,因为您不关心的所有事情现在都由某个机制负责。 (同样,我们有一个很好的提示数字的机制,以后可以重复使用。)

甚至更好:也许有更好的方式显示提示并获取数字。如果我们决定改为编写GUI,则可以更改机制代码而无需更改策略代码。策略代码并不在乎如何选择有效手势,而只是在乎。

评论


\ $ \ begingroup \ $
很好地解释了这一点,将封装和责任分离的思想放到了其他人更容易理解的更具体的术语中。
\ $ \ endgroup \ $
– UpAndAdam
2014年10月10日22:39

\ $ \ begingroup \ $
我通常不喜欢while循环,而喜欢while循环。我想我仍然可以将它编写为while循环。完成更改后,我会将代码发布到游戏中。
\ $ \ endgroup \ $
–马拉奇♦
2014年3月11日在2:41

\ $ \ begingroup \ $
我知道通常不建议留下“ +1”和“谢谢”的注释,但是我想在此忽略它,只是想感谢Eric,感谢您在Stack Exchange中的所有回答。我会定期浏览您的个人资料上的最新答案(主要是关于SO的信息),因为您提供的信息的质量始终很高。
\ $ \ endgroup \ $
–约翰H
2014年11月11日12:47

\ $ \ begingroup \ $
@JohnH:我认为这种挫败感适得其反。礼貌花费很少,而且在使人们对寻求帮助和向他人提供帮助方面都感觉良好。不客气这是我的荣幸。感谢您的客气话。
\ $ \ endgroup \ $
–埃里克·利珀特
2014年11月11日13:14

\ $ \ begingroup \ $
@EricLippert“谢谢-关于X的要点”评论很好;只是“谢谢”会是噪音。投票是使Stack Exchange网站正常运行的感谢之本,所以我们更喜欢投票-这是最有可能的感谢形式。我注意到您到目前为止只投票了两次。通过让像您这样的专家通过投票识别其他成员的良好问题和答案,Code Review将大为受益。
\ $ \ endgroup \ $
– 200_success
2014年5月7日下午4:30

#2 楼

随机Nitpicks


validInput应该命名为isValidInput,以遵循bool的命名约定。 listOfGestures会是一个更好的名字。
如果List<Gesture>gestures,则检查值是否在边界内的条件将变得更加简单:
我是否需要创建一个新方法来整理用户输入以及它是否在最小和最大范围之内?

如果您全程OOP并将该逻辑封装到自己的对象中,您可以编写一个单元测试,以验证有效输入是否返回gestures:或任何合法输入的Dictionary<int,Gesture>实例。这会稍微改变您的代码:
interface IGestureInputValidator
{
    Gesture ValidateInput(string userInput);
}

注意gestures.TryGetValue(playerChoice, out playerGesture);是一个实例字段;您可能可以将该实例注入到构造函数中(或在其中插入Gesture)。
在验证器界面中进行了一些小的更改:
Gesture playerGesture;
while (playerGesture == null)
{
    Console.WriteLine("Please choose your Gesture:");
    var input = Console.ReadLine();
    playerGesture = _validator.ValidateInput(input);
}

但是我不喜欢这样,因为意图已经不那么清晰了。一个null可以是任何东西,我们正在寻找一种特定的字符串。我认为手势应该属于自己的职业,甚至更好。无论您做什么,将验证提取到其自己的对象(或方法中,如果您将责任视为持有其他逻辑的同一类的一部分),则可以更改该逻辑,而不会影响其余代码。 br />建议您尽可能将问题分开,以便大胆回答您的问题,是的,您应该将其分开。

评论


\ $ \ begingroup \ $
在一种情况下,您要告诉他使用bool的命名约定来指示类型,在另一种情况下,则建议他不要在名称中表明其变量是列表。这不是矛盾的吗?
\ $ \ endgroup \ $
–内特C-K
2014年3月10日20:53

\ $ \ begingroup \ $
@ NateC-K bool是具有true / false值的简单值类型; “ IsXxxxxx”是它们的既定命名约定。列表是实现接口的引用类型。通过在变量的名称上加上“列表”,您可能将猫称为狗,...或狮子。许多类型实现IList ,而不仅仅是List ;调用一个可枚举的xxxxList是一个谎言。
\ $ \ endgroup \ $
– Mathieu Guindon♦
2014年10月10日21:56

\ $ \ begingroup \ $
“ is ...”命名约定是匈牙利表示法的一部分,MS表示您不应再为它烦恼。任何实现IList的东西都必然是IList(更不用说此类型的变量只能用于调用实现IList的方法),所以我看不出如何称呼它为“谎言”。清单。您的程序只关心当前抽象级别的变量类型;您似乎建议我即使在无关紧要的情况下也应该关心抽象的基础细节。
\ $ \ endgroup \ $
–内特C-K
2014年3月10日22:09

\ $ \ begingroup \ $
变量命名仅适用于Developer,无论您使用什么名称,编译器都会编译代码。标准适用于维护代码的人员。命名方案作为指导原则。
\ $ \ endgroup \ $
–马拉奇♦
2014-03-10 22:29



\ $ \ begingroup \ $
@ NateC-K,来自Microsoft的类型成员名称(4.5):“ 1.用多个短语描述集合中的项目,而不要使用单数形式的短语“ List”或“ Collection”。 “ 2.请使用肯定性短语(CanSeek而不是CantSeek)来命名布尔属性。可选地,您也可以在布尔属性前加上“ Is”,“ Can”或“ Has”作为前缀,但只能在增加值的地方使用。
\ $ \ endgroup \ $
–白曼
2014年11月11日15:18



#3 楼

最好是出于模块化的考虑,并且将这整段代码放置在单独的方法中可重用。如果除了他们想要使用什么手势之外,还需要更多用户输入,还可以使其更通用,以便在各种情况下在程序的其余部分中重复使用。

您的代码似乎在使用playerChoice变量时有点不合逻辑,即使您知道未正确解析它也是如此。您设置了validInput,但随后不要在if语句中进行检查。

注意:我还将playerChoice的默认值更改为0,因为在这种情况下,这显然超出了范围,因为您说有效输入必须超过0。是次要的和不必要的,但我只想让您注意它。可以向用户提供特定反馈的代码,以告诉用户输入错误的原因(我的评论中的第9点)。如果您不希望在方法中找到此类反馈,请使用以下简单方法,该方法可从控制台获取Integer User输入。

评论


\ $ \ begingroup \ $
很好,但是您的代码存在如下错误:输入0或比手势数高的数字仍会结束循环。诚然,这也是原始代码中的一个错误,但在那里更明显。
\ $ \ endgroup \ $
–鲍勃森
2014年10月10日18:42

\ $ \ begingroup \ $
@Bobson我不确定我是否看到问题,如果用户输入0,它将使playerChoice> 0失败,这会将validInput设置为false,这将导致主循环再次执行。
\ $ \ endgroup \ $
– BenVlodgi
2014年10月10日在18:45

\ $ \ begingroup \ $
哦,不,你是对的。我在读取()时放错了位置,因此只能将有效输入设置为TryParse()的结果,而不是其余条件。
\ $ \ endgroup \ $
–鲍勃森
2014年10月10日19:15

\ $ \ begingroup \ $
@Malachi是的,所有操作都是按顺序进行的,因此只有在TryParse没有失败的情况下才进行比较。 TryParse必须完成才能确定这一点,这意味着它在继续之前已经发出了playerChoice。
\ $ \ endgroup \ $
– BenVlodgi
2014年10月10日19:26



\ $ \ begingroup \ $
如果您将大量逻辑塞进do-while块的测试部分,那么我认为它会使代码更难阅读。我认为通常将更好的做法是将诸如读取输入之类的内容放入循环体内,前提是这样做的结果相同。
\ $ \ endgroup \ $
–内特C-K
2014年10月10日20:40