Critique and feedback for my little project wanted.

xuanbao · · 596 次点击    
这是一个分享于 的资源,其中的信息可能已经有所发展或是发生改变。
<p>Hello everyone,</p> <p>I was looking for a new programming language for a while and when I found Go I feel in love rather quickly. As practice I am writting a small command line tool which is supposed to help me with my todo list. So far I only allows adding and printing todos for certain dates and setting them to done. In the very near future I want to add further functionality such as deleting todos entirely or editing their description.</p> <p>I have developed Java for almost 10 years and as a result I feel like I am writting a very Java like go (e.g. very long variable names etc.). So if you could have a look at my code and tell me what things are not really idiomatic for go or just not go like and possibly how I could do better, i would really appreaciate it.</p> <p><a href="https://github.com/FChris/godo" rel="nofollow">https://github.com/FChris/godo</a></p> <p>Thanks and all the best</p> <p>Edit: I incorporated most of the changes suggested by <a href="/u/adamcolton" rel="nofollow">u/adamcolton</a>. So you might that the stuff he is explaining is not there anymore. At the time he was looking at it the repo it was at commit <a href="https://github.com/FChris/godo/tree/227456dd6c42198a79e8b095c551ae5c9fa369df" rel="nofollow">227456d</a><br/> Thanks again <a href="/u/adamcolton" rel="nofollow">u/adamcolton</a></p> <hr/>**评论:**<br/><br/>adamcolton: <pre><p>Good project, a few notes;</p> <p>I would embed the Reader in Scanner rather than giving it a named field. Often, if a struct has a single field, that&#39;s a sign it should be either an alias or embedded. In this case, it should be embedded because you want access to the methods on the reader. As a bonus, you&#39;ll be able to drop your unread method and just call UnreadRune directly on the scanner. Also, Scanner isn&#39;t used outside the parse package so I wouldn&#39;t export it (change it to &#34;scanner&#34;).</p> <p>The parse package would also be a great place to add some tests. And testing is pretty tightly integrated with Go for a reason, it is idiomatic. Don&#39;t worry about getting perfect test coverage, just get enough to show how the code works and test some of the main paths. You may want to look at <a href="https://godoc.org/github.com/stretchr/testify/assert" rel="nofollow">assert</a>, it&#39;s a good test helper package.</p> <p>You could also have your Token constants be runes (and the Token type be a rune) which could simplify some of the switch logic.</p> <p>In parse.go, you&#39;ve got the question &#34;What are we actually doing here?&#34;, not sure if you meant that literally, but I&#39;ll answer and hope it&#39;s useful. The &#34;task.Day{}&#34; part is initializing a Day struct with everything set to it&#39;s zero value. And the &amp; takes the address, returning a pointer. In this case, I wouldn&#39;t use a pointer. Through the rest of the code, Day is not a pointer, so I&#39;d make it consistent. After that, I&#39;d change the line to</p> <pre><code>var taskDay task.Day </code></pre> <p>Which is idiomatic (when initializing a zero value, non pointer object).</p> <p>In the task package, TodoList and DayList should both be type aliases.</p> <p>The last thing I&#39;d recommend is to grab <a href="https://github.com/golang/lint" rel="nofollow">golint</a> and use that.</p> <p>I hope that&#39;s helpful.</p></pre>CreativeCoconut: <pre><blockquote> </blockquote> <p>Hello there,</p> <p>Thank you for the comments, this helps a lot.</p> <p>I did not know that you simply could embed single fields but a quick search gave me a good idea what it is. That sounds really good.</p> <p>I think writing tests is the next thing I will do, before adding the remaining functionality I want. </p> <p>The commend &#34;What are we actually doing here?&#34; was meant quite litereally as at the time of writting I did not understand why I needed a pointer there. I kinda figured it out now and just forgot the comment but I also think your suggestions are a big improvement.</p> <p>I will also have a look at golint :-)</p> <p>Thank you for your time, it helps tons. I appreciate it a lot :-)</p></pre>CreativeCoconut: <pre><p>One more question.<br/> If I make TodoList and DayList aliases instead structs the way I made them, I cannot change them in place in functions like &#34;InsertTodo&#34; or is there another way I am missing?</p></pre>adamcolton: <pre><p>In order to modify them, you&#39;ll need something like this: <a href="https://play.golang.org/p/vwnmnUHRWW" rel="nofollow">https://play.golang.org/p/vwnmnUHRWW</a></p> <p>but beware of this <a href="https://play.golang.org/p/lfdvrQpfX1" rel="nofollow">https://play.golang.org/p/lfdvrQpfX1</a></p> <p>The other option is to have any method that modifies the slice return a slice, which matches the behavior of append. But it&#39;s a matter of taste and circumstance.</p></pre>CreativeCoconut: <pre><p>The behavior of append was what I started out with but I did not like the feel of it as I felt like I manually had to copy in different places even after I inserted my new Element.</p> <p>Can the second behavior be explained by saying that everything is call by value in go and that you can only mimic call by reference when you use pointers? Is there such a thing as call by reference in Go?</p> <p>Thanks for the examples</p></pre>

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

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