request for (very small) code review

agolangf · · 437 次点击    
这是一个分享于 的资源,其中的信息可能已经有所发展或是发生改变。
<p>I am new to golang. Employer send me a test exercise and turned me down without further explanations.</p> <p>I&#39;ll appreciate if someone would tell me what&#39;s wrong with my code, there&#39;re only 110 lines of it. Here&#39;s a link with test description and implementation:</p> <p><a href="https://gist.github.com/dmokhov/6539a2d920dec7e140ba02ea6f917ed8" rel="nofollow">https://gist.github.com/dmokhov/6539a2d920dec7e140ba02ea6f917ed8</a></p> <p>If it&#39;s not right place to ask for such help, please advise more appropriate.</p> <hr/>**评论:**<br/><br/>driusan: <pre><p>If I were looking for something to say in code review, I&#39;d say get() should return (int, error) instead of int. But on the whole, I&#39;d say it&#39;s far above the level of quality I&#39;d expect on a test from a job application who doesn&#39;t claim to know Go..</p></pre>peterbourgon: <pre><p>I agree. If the clause at the top of the program was the entire challenge description, then this is a good submission. I could come up with some nits around error handling, perhaps a bit of structural improvement... but nothing serious.</p></pre>dchapes: <pre><p>Line 39: <code>Timeout</code> is a <code>time.Duration</code>, for a one second timeout you should use <code>Timeout: time.Second,</code> (and for a 10 second timeout you should use <code>Timeout: 10 * time.Second,</code>).</p> <p>Line 51: I&#39;d avoid reading the entire (possibly huge) response body into memory with <code>ioutil.ReadAll</code> and instead count occurrences while reading/streaming the body.</p> <p>Lines 77 &amp; 84: you use <code>defer</code> for <code>wg.Done()</code> but put <code>&lt;-sem</code> at the end where it won&#39;t get run if something should panic. I&#39;d put both cleanup together (either as <code>defer</code> [preferred] or at the end of the function).</p> <p>Line 81: IMO this belongs at line 104; it&#39;s cleaner that way and writing to <code>os.Stdout</code> via <code>fmt.Printf</code> may not be safe in concurrent gotroutines.</p></pre>j7b: <pre><p>Basically this code review stops at &#34;program doesn&#39;t provide specified output.&#34;</p> <pre><code>fmt.Println(&#34;Reading from stdin...&#34;) </code></pre> <p>Adding things like output that&#39;s not in the specification is the quickest way to fail these things. Things like adding the timeout can also backfire.</p> <p>Using a goroutine in gen() is (arguably) a violation of the k constraint; if somebody evaluating the work thinks you&#39;re sidestepping the specification that can also sink you.</p> <p>I also agree with the points about buffering the entire response and fmt.Printf not best for concurrent use.</p> <p><a href="https://play.golang.org/p/cyGuDG-u62" rel="nofollow">https://play.golang.org/p/cyGuDG-u62</a> might be closer to what they were looking for but I find the exercise somewhere between ambiguous and poorly-worded.</p></pre>

入群交流(和以上内容无关):加入Go大咖交流群,或添加微信:liuxiaoyan-s 备注:入群;或加QQ群:692541889

437 次点击  
加入收藏 微博
暂无回复
添加一条新回复 (您需要 登录 后才能回复 没有账号 ?)
  • 请尽量让自己的回复能够对别人有帮助
  • 支持 Markdown 格式, **粗体**、~~删除线~~、`单行代码`
  • 支持 @ 本站用户;支持表情(输入 : 提示),见 Emoji cheat sheet
  • 图片支持拖拽、截图粘贴等方式上传