First Go program. Would like a code review.

agolangf · · 452 次点击    
这是一个分享于 的资源,其中的信息可能已经有所发展或是发生改变。
<p>I wrote an <a href="https://github.com/kalbhor/Image-Scraper" rel="nofollow">Image Scraper</a> in go.</p> <p>I tried to implement concurrency.</p> <p>I would love to know what I can improve/best practices. </p> <hr/>**评论:**<br/><br/>Ballresin: <pre><p>I&#39;d only suggest you create your own http.Client and set a readtimeout, otherwise you might see some very long wait times while connections just dangle. The default client will literally wait forever.</p></pre>SeerUD: <pre><p>1) Definitely take a look into enabling <code>gofmt</code> or <code>goimports</code> support in your editor (if available), or get something that can trigger it on-save. I can see some style issues in your code (mixed indentation for example).</p> <p>2) Local variables usually use camelCase not PascalCase.</p> <p>3) Comments like these are pointless:</p> <p><code> var wg sync.WaitGroup // Used for waiting for channels </code></p> <p>Also, you&#39;re not waiting for the channels here, you&#39;re waiting for the goroutines. You could probably also use channels to do the waiting too, they also block.</p> <p>4) In <code>DownloadImg</code> you&#39;re deferring right at the end of the function. You don&#39;t need the defer there. Putting it at the top would make sense though, from both a readability and use-of-defer standpoint.</p> <p>5) Could you replace <code>resp.Find(&#34;*&#34;)</code> with <code>resp.Find(&#34;img&#34;)</code>? It looks like it could speed things up.</p> <p>6) Have you considered the how testable this code is? It can be very tricky when working with IO, but I&#39;ve recently learned it&#39;s quite an important area to learn how to test effectively.</p> <p>7) Do the function&#39;s you&#39;ve exposed need to be exposed? Probably not.</p></pre>Djent_: <pre><p>Your variable names could use some work, specifically in SliceUniq and when you dispatch your downloaders. Try renaming &#34;l&#34; to &#34;min&#34; and &#34;i&#34; to &#34;max&#34; there. As other&#39;s have mentioned, run gofmt</p></pre>readytoruple: <pre><p>Cool. One thing that jumps out is:</p> <p>You are currently waiting on your slowest URL to finish scanning before you start downloading, why not dispatch the downloaders as soon as img uris are scanned?</p></pre>Djent_: <pre><p>They are checking to make sure all urls are unique once they have all been crawled, so they can&#39;t start downloading unless they know it is a unique url. However, they can check the url before adding it to Images, which may be more efficient.</p></pre>nnunley: <pre><p>Why not use a map of url to boolean. use that to gate whether to fetch the image or not.</p> <p><a href="https://blog.golang.org/go-maps-in-action" rel="nofollow">https://blog.golang.org/go-maps-in-action</a></p> <p>Then you can have one goroutine scraping urls while you have another one (or more) fetching the images. Set the map for the src to true or false just before fetching (you might want a mutex to protect the access)</p></pre>poxopox: <pre><p>You should probably handle that first error immediately after you call that function</p></pre>

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

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