这个问题是职业问题和代码审查之间的交叉。我不确定在哪里问,但是由于涉及到代码,所以我使用了CodeReview。试图弄清我的反馈意见,以改善我的体验。解决方案太复杂(没有像Big O那样谈论复杂性)。我正试图了解这意味着什么,所以我知道如何以及要进行哪些更改。是高级的,也许还不止这些。所有的讨论都是围绕建筑和复杂的问题展开的,当我发现自己在思考“什么是最好的设计等”时,这可能会困扰我。需要您的帮助):
过度思考/过于复杂意味着什么?有示例吗?
什么是避免这种情况的好方法?

我举一个例子,请记住代码是在压力下编写的,我是神经残骸。我必须实现的一件事是用0替换电话号码中的国家/地区代码,并删除空格和非didgit(这是大约10个用户案例中的一个)。我做了TDD风格的测试,这里是测试:

        [TestMethod]
    public void StripNumber_WhenGivenNumber_ReturnsNumberWithoutSpaceAndNonNumeric()
    {
        var number = "046 55.5";
        var expectedString = "046555";

        var numberManager = new PhoneNumberManager(number);
        var cleanedString = numberManager.StripWhiteSpaceAndNonDigit().Result();

        Assert.AreEqual(expectedString, cleanedString);
    }


以及经过快速重构的代码。
我做了什么/如何做:


从测试开始
编写了足够的代码以使其通过–中间进行了一些红色测试,进行了调试
重构为类
经过验证的测试仍然可以运行绿色
快速的次要重构
转到下一个故事

花了20分钟讨论了它,并讨论了其他问题(如果我是在工作和编码而不是面试的话,我认为这样会少一些)

请记住,这是实时的通过屏幕共享进行编码,这只是要实现的10个故事之一。分享这个让我很痛苦,但这就是我的代码在压力下的样子。

编辑:由于有些人似乎错过了一些用户故事的一部分:共有10个,全部以某种方式处理电话号码。

编辑RE。 TDD:面试官在面试之前就TDD谈论了很多(以及这对他们的重要性),在我开始编码之前,我问我是否应该进行TDD,因为他们曾说过我将接受代码和测试的评估。答复是:“做平时要做的事”-所以我做了TDD。我们将要使用的数据,因此我可以估算出我应该为复杂性担心多少。
这里的命名并不可怕,但由于代码是重点,因此我将暂时保留它(我将其删除了)
我不喜欢Regex和Replace组合,我应该只使用Regex
另一个开发人员(面试)建议进行Regex比赛,但我说不,因为那意味着我们会得到在每个非数字上分割,产生一个我认为会增加开销的集合。
我不喜欢孤独的“ 0” concat
我很后悔没有使用字符串生成器,但是数据量却没有大(我问的第一件事)

在编码会议结束时,我将类重构为一种使用方法链的流利方法,并解释说我认为这是一种流水线方法,我们可能想使用不同的管道和标准我有一个定义的位置要输出。

我哪里出错了?代码中过度思考的迹象是什么?-而且,总是很高兴听到代码中有什么混乱或可以改进的内容:)

评论

我注意到的第一件事是您有一堂课,名字以Manager结尾。

是的,命名时我完全空白了。我很想在此处发布内容时更改所有内容,以节省面子,但是老实说,这就是我在这种情况下编写代码的样子。肯定不是很漂亮:)

你会做代码katas吗?

是的,我几乎每天都会这样做。我在kata,koan和阅读代码之间切换。为此,我每天早上在日历中预留1h的时间,在晚上还有1 h用于读取代码的时间。我真的很喜欢代码:)

您所说的“用户故事”到底是什么意思?欢迎使用Code Review,恭喜您在首次尝试时遇到了一个热门问题!

#1 楼

过度考虑的唯一迹象是您使用了业务原语(PhoneNumberManager)。

该问题要求您以字符串开头并以字符串结尾,因此中间不需要额外的类型

它似乎也使您错过了显而易见的字符串扩展方法。

在采访中弄错Regex通常是可以的,因为几乎没人能完美地写出Regex的头。这方面的一个改进是不必在每次调用时都重新创建正则表达式,而应使用静态正则表达式。

评论


\ $ \ begingroup \ $
我不认为应该采用扩展方法(尽管我不经常使用C#),但是我同意管理器类是过大的。
\ $ \ endgroup \ $
–西蒙·福斯伯格
2014年7月4日在21:15

\ $ \ begingroup \ $
该类具有更多逻辑(如我在问题中所写,总共有10个用户案例,所有相关)。我只发布了一种方法和测试,因为我需要与公司确认我可以发布整件事。
\ $ \ endgroup \ $
–鸢尾花
2014年7月4日在21:16



\ $ \ begingroup \ $
@irisclasson您还有什么其他领域?如果只是字符串,那么我的意见会成立。
\ $ \ endgroup \ $
– Graeme Bradbury
2014年7月4日在21:23

\ $ \ begingroup \ $
一种针对字符串的扩展方法,该方法仅对一个字符串有用,并且仅对一个字符串用例有用,应该首先封装在其自己的类中?哦,不,我不会认为这是非常糟糕的设计(对每个字符串都设置RemoveDialingCode有意义吗?几乎没有)。诚然,我只是调用类Phonenumber,在构造函数中将字符串作为输入并在那里正确处理。
\ $ \ endgroup \ $
– Voo
2014年7月5日在17:07



\ $ \ begingroup \ $
+1是因为:“过度思考的唯一迹象是您对业务原语(PhoneNumberManager)的使用。”这是PhoneNumberManager的想法,而不是有关类的名称的想法。有了这个想法,然后看到类保持状态对我来说就足够了。这就是为什么我也喜欢建议的扩展方法的原因。直接,无状态。没有引入新的设计概念。无论较大的设计是什么样的,扩展都可以很好地发挥作用。我认为对实现细节的任何讨论都是没有意义的。
\ $ \ endgroup \ $
– radarbob
14年7月13日在1:19

#2 楼

Regex表达式[^0-9.]匹配09.以外的任何内容。
如果从表达式中删除.,它应该只匹配您想要的所有内容。除了数字以外的任何东西。

所以整个事情可以写成一行

值得创建一个\D类,该类保存执行此类任务的直接(可能是临时的)结果。可以将其重构为这样的辅助类:

Regex.Replace(value, @"\D", "")




预优化是万恶之源。 br />
虽然您的代码可能不是完美的或未优化的,但是它可以工作,这是您的主要关注点/目标。优化和重构之后才出现,因为在处理代码时,您对问题的看法变窄了。而且,因此无法查看大图。

刚开始作为初级开发人员时,请不要过分推销自己。许多工具和技术都是从经验中提炼出来的,您最终会沿途学习。

评论


\ $ \ begingroup \ $
是的,回想一下主要的脸部护理。我对自己的正则表达式功能感到压力很大,做了一个.Replace以继续进行下一部分。
\ $ \ endgroup \ $
–鸢尾花
2014年7月4日在20:40

\ $ \ begingroup \ $
该网站应该可以帮助您可视化正则表达式:regexper.com/#%5B%5E0-9.%5D
\ $ \ endgroup \ $
– Xiaoy312
2014年7月4日在20:49

\ $ \ begingroup \ $
如果指定了ECMAScript兼容行为,[^ 0-9]仅等于\ D,请参阅正则表达式选项中的“ ECMAScript匹配行为”部分。
\ $ \ endgroup \ $
– Johnbot
2014年7月7日在11:33

\ $ \ begingroup \ $
这很臭:如果我采取您的方法,最终会带来大量的物体和半吨的助手。助手表明OOD不好。大多数情况下,您会使用Domain对象(例如address),这应该是其中的一种方法。您的帖子的其余部分似乎合理有效,
\ $ \ endgroup \ $
–托马斯垃圾
2014年8月9日14:47

#3 楼

在进行面试时,很少有面试官期望您提出理想的解决方案。主要是,他们正在寻找有关您如何思考以及如何解决问题的想法。特别是作为初级程序员。

首先从测试驱动的角度解决问题似乎是合乎逻辑的方法,但是仅编写代码来解决问题,然后坚持测试驱动的方法,直到被问到如何解决问题时,该怎么办?让它变得更好,让对话来解决。有些地方仍然不了解测试驱动的真正含义,因此您可能会觉得太高级了。如果您想了解他们是否重视这种技能,请向他们提出有关他们发展过程的问题。 ”这句话。您已经建立了一个完整的工具箱,并且用了相对较短的时间,因此应用其中的某些东西自然会成为您的自然选择。 br />
尝试着手解决问题的基础架构
当您必须考虑解决问题所需的nuget软件包时,
基本上,每当您的思维偏离问题时已经受命

雇主正在寻找开发人员来解决问题。因此,他们很想知道您是否可以接受需求,运行需求并将其转化为解决方案。

评论


\ $ \ begingroup \ $
如果我一直在接受采访,我可能会说:“太好了,您正在编写测试!但是对于此任务,如果您愿意,我们就直接跳到代码上。”
\ $ \ endgroup \ $
– David Harkness
2014年7月4日在21:28

\ $ \ begingroup \ $
我问我是否应该采用TDD方法,因为招聘经理说过“您将与X配对,我们可以看到您的代码和测试技能”。当我问其他开发人员是否应该进行TDD时,他说:“照常做”。所以我去了TDD。
\ $ \ endgroup \ $
–鸢尾花
2014年7月4日在22:31

\ $ \ begingroup \ $
“做平常的事情”通常可以说是“这不是我们平常的事情,但是可以尝试告诉我们我们应该做不同的事情”。
\ $ \ endgroup \ $
–Rowland Shaw
2014年7月5日在7:58



\ $ \ begingroup \ $
好答案!也欢迎使用CodeReview,期待阅读更多答案!
\ $ \ endgroup \ $
–马拉奇♦
14年7月13日在13:21

#4 楼

可能更容易阅读:

    public static string GetFormattedPhoneNumber(string input)
    {
        StringBuilder result = new StringBuilder();

        foreach (char character in input)
        {
            if (Char.IsDigit(character))
            {
                result.Append(character);
            }
        }

        return result.ToString();
    }


评论


\ $ \ begingroup \ $
欢迎使用代码审查!您的答案在“第一篇文章”队列中。当他们也解释了一些更改内容时,这里的答案往往会更好,而不仅仅是“此代码可能更好”
\ $ \ endgroup \ $
–西蒙·福斯伯格
2014年7月4日在21:06

\ $ \ begingroup \ $
元审查:每当您遍历一个集合以滤除某些内容并可能转换元素以生成一个新的集合时,Linq通常是首选工具。您的代码可以缩短为一行:return new string(input.Where(Char.IsDigit).ToArray());
\ $ \ endgroup \ $
–ChrisWue
2014年7月5日在7:16



\ $ \ begingroup \ $
欢迎使用CodeReview!这个问题的好简单答案。我认为克里斯说的最好。而且我不知道什么效率更高,我假设来自op和此答案的所有解决方案都以一种或另一种方式使用循环
\ $ \ endgroup \ $
–马拉奇♦
14年7月13日在13:28

#5 楼

您的正则表达式会删除非0-9或“。”的任何内容,然后替换为“。”。您可以删除“。”从正则表达式中一次性完成所有操作:

“ [^ 0-9]”而不是“ [^ 0-9。]”

请记住以确保回答提出的问题。您已经简化了数字,但是要求是获取国家/地区代码。

在技术问题中,我倾向于关注能够提供所要求答案的最小数量的代码。较小的代码使审阅者更容易理解(如果您正在做一些棘手的事情)。

我不会采用TDD方法。我只有一个可以调用以产生输出的函数(也许带有支持的函数或类)。

测试方法可以测试您的函数,但最重要的是实际回答问题,并证明他们正在寻找的结果。

表明您知道超出问题范围的东西听起来像是一个好主意,但它可能使您的解决方案晦涩难懂,并可能使您迷失方向一个更简单的解决方案也许可以解决问题。

评论


\ $ \ begingroup \ $
对不起,我写的时候忘记了两个用户故事是合并在一起的。问题是两个部分,将国家代码替换为0,并删除空格和非数字。香港专业教育学院更新了问题
\ $ \ endgroup \ $
–鸢尾花
2014年7月4日在20:27



#6 楼

return this;对我来说是一个主要的“过度思考”,我个人主要将其用于Builder模式。由于您的班级名为PhoneNumberManager,听起来不像PhoneNumberBuilder(可能重命名吗?)。您提到了您还有其他方法,所以主要问题是:链接它们是否有意义?还是只需要调用其中一个或两个然后获得结果? :

    var numberManager = new PhoneNumberManager(number);
    var cleanedString = numberManager.StripWhiteSpaceAndNonDigit().Result();


然后var cleanedString = PhoneNumber.StripWhiteSpaceAndNonDigit(number);会更好,更易读且更简单。方法返回string并要求string可以使用Func<string, string>委托。然后,我相信您在PhoneNumber类中可以有很多静态方法,并使用一种实用程序方法: br />我可能在这里混合了C#和Java语法。 (旧的好习惯很难治好)


我不得不说,尽管您似乎了解您的C#,但您遵循的是我所知道的所有C#约定,当然除了PhoneNumber.Process(PhoneNumber::StripWhiteSpaceAndNonDigits, PhoneNumber::StripStuff)应该是private string _nonNumeric = "[^0-9.]";readonly

评论


\ $ \ begingroup \ $
这并不简单。 C#约定用于使代码按您阅读的顺序执行(从左到右)。通过内联这些方法调用,代码将从右到左执行。还要想象一下,如果所有十个方法都以这种方式链接在一起,那么您将以十个小括号结尾,那么代码将是什么样。
\ $ \ endgroup \ $
–格雷姆·布拉德伯里(Graeme Bradbury)
2014年7月4日在22:10

\ $ \ begingroup \ $
@GraemeBradbury这就是为什么我说“简单”的原因,我同意这不是一个干净的解决方案。我发现这个问题虽然缺乏背景。
\ $ \ endgroup \ $
–西蒙·福斯伯格
2014年7月4日在22:14

\ $ \ begingroup \ $
@GraemeBradbury我用另一种方法更新了答案,您是否更喜欢这种方法?
\ $ \ endgroup \ $
–西蒙·福斯伯格
2014年7月4日在22:41

#7 楼

不知道这是否是问题,因为我当时不在那儿(错过了所有交流),但是:

如果要求是:


我要做的事情之一是将电话号码中的国家/地区代码替换为0,并删除空格和非数字(这是大约10个用户案例中的一个)。


测试用例,您必须解释要求:乘以0 ?,这是由另一种方法完成的?)

如果我阅读了要求并将其写成一个测试,我最终得到:

br />测试代码的编写方式无关紧要,但要看与对象交谈的区别。

(我们可以整日争论静态方法的好坏,但这就是不是我想指出的。)

您没有对象来描述电话号码,但是有一些瑞士军刀班可以做些什么er,您要求输入一个字符串/电话号码(我不知道这是否是必需的)。

您无法询问:您能给我这个字符串的本地号码吗?

您需要提出以下问题:是否可以为我替换空格并删除非数字字符,还可以将该字符串的区号替换为0?

我不知道吗?不知道在给定的问题域中什么是重要的,但可能仅获取本地号码就足够了。那么其他一切都可能被认为是在思索吗?

#8 楼

以下是一些我不想作为面试官的几点:


测试方法的名称非常抽象,只需将其命名为Test

名称PhoneNumberManager对此令人困惑,PhoneNumberHelper会更合适>没有理由返回this,因为显然调用者已经知道它了。
您正在使用正则表达式修改数据,而您甚至不确定一开始数据是否有效,但我只是假设面试官没有明确要求。
您正在执行其他String.Replace操作,您可以立即通过Regex.Replace完成操作,


#9 楼

关于类设计,您已经有了很多不错的答案。 (这应该是一个静态方法,将字符串作为参数并返回一个字符串,别无其他,但是您已经被告知)。

所以,让我专注于regexp设计。这不是正则表达式问题,您有一个非常简单的文本处理任务。当需要获取具有不同长度的字符串,替换字符串而不是字符等时,请对复杂的字符串使用正则表达式。在这里,您只需用它来过滤字符串。由于字符串在C#中是不可变的(在这种情况下,这很可悲,因为否则您可以更改字符串,因为生成的字符串总是一样长或短),您需要创建一个新字符串,其中只包含一些字符。是的,是旧版本。 br />顺便提一句,任务也是删除您没有的国家代码。我想您已经做到了,但是这里没有提供代码。否则,那肯定会是一个巨大的怀念。

#10 楼

string Digits = (new PhoneNumberManagerFactory() as IPhoneNumberManagerFactory).Create<PhoneNumberManager>().StripNonDigits("123.456");


这可能是这种问题有点“过度思考”的示例。但是,我喜欢抽象工厂模式,因为您可以轻松地向其添加功能。

评论


\ $ \ begingroup \ $
欢迎使用代码审查!我担心您的答案与此处的格式不太吻合。我们假设要复查问题代码,而不要过多解释就不建议这样做。
\ $ \ endgroup \ $
–马克·安德烈(Marc-Andre)
2014年7月8日13:21