Reassigning variables [Code Review]

blov · · 362 次点击    
这是一个分享于 的资源,其中的信息可能已经有所发展或是发生改变。
<p>I wrote some kind of countdown in Go, that reads date from file (in form of day.month.year) and calculates amount of days, hours, minutes, seconds between now and the specified date in file.</p> <p> </p> <p>Since I come from Python and Js background, I am used to do things like:</p> <pre><code>// this is a stupid example, since I could do that in the same row but keep reading days = 10 days = str(10) </code></pre> <p>I like to do things this way, so I don&#39;t need to name another variable that will contain the same data. Well Go does not allow to do things this way. That&#39;s fine, but the question now is, how do you form your programs to get around this problem? Do you add numbers to the end of the variable names (like days2)? Or you just name them whatever, and I am worrying too much about this thing.</p> <p> </p> <p>I am adding my program below as an example for this problem (see line with #####). E: since it&#39;s hard to read without syntax highlighting I am pasting it here as well: <a href="https://play.golang.org/p/n8HmUZOqq7" rel="nofollow">https://play.golang.org/p/n8HmUZOqq7</a> or <a href="https://paste.ubuntu.com/23416481/" rel="nofollow">https://paste.ubuntu.com/23416481/</a></p> <hr/>**评论:**<br/><br/>WintyBadass: <pre><p>You can return them without declaring new variables:</p> <pre><code>return fmt.Sprintf(&#34;%v&#34;, days), prefixNumber(hours), prefixNumber(minutes), prefixNumber(seconds) </code></pre> <p>or if this is too ugly/long for you just to it same way as in your example. It doesn&#39;t matter. You won&#39;t pollute scope with them, because right after declaration scope ends and compiler will optimize them away anyway.</p> <p>Or you could do this: (no don&#39;t do this its a joke, but working :)</p> <pre><code>var val interface{} val = &#34;test&#34; fmt.Println(val.(string)) val = 10 fmt.Println(val.(int)) </code></pre></pre>shovelpost: <pre><p>Some suggestions:</p> <blockquote> <p>Do you add numbers to the end of the variable names (like days2)? Or you just name them whatever, and I am worrying too much about this thing.</p> </blockquote> <p>In Go we favor short names for very short lived variables. For example you could have:</p> <pre><code>d := diff / 86400 diff = diff - (d * 86400) h := (diff / 3600) diff = diff - h*3600 m := diff / 60 diff = diff - m*60 s := diff days = fmt.Sprintf(&#34;%v&#34;, d) hours = prefixNumber(h) minutes = prefixNumber(m) seconds = prefixNumber(s) </code></pre> <p>Notice that instead of <code>days</code>, <code>days2</code> or <code>rdays</code>, someone could go for <code>d</code> and <code>days</code>, since <code>d</code> only lives in this short function.</p> <p>Something else that could help you is <a href="https://golang.org/doc/effective_go.html#named-results" rel="nofollow">named result parameters and naked return</a>.</p> <p>This is something that we usually avoid but it is very helpful for short functions when it benefits clarity.</p> <p>For example, seeing a function that returns <code>(string, string, string, string)</code> is not very clear what each string is. You could instead make it:</p> <pre><code>func calculate(...) (days, hours, minutes, seconds string) { </code></pre> <p>This is much more readable as you can easily see that the function returns days, hours, minutes and seconds which are all of type string.</p> <p>When you use named result parameters, those names are already defined as variables in the function so you do not need to redefine them with <code>:=</code>. Also in the end you can simply write <code>return</code> which will simply return those named result parameters (and that&#39;s a naked return).</p> <p><em>EDIT: I want to emphasize that this is a special case where named result parameters seem to benefit the clarity of the code as they act as documentation. Unless you have a similar case, you should avoid using them. This also goes for &#34;naked&#34; returns.</em></p> <p>Another suggestion is instead of doing all this manual work of parsing the string date yourself, make your function accept <code>time.Time</code> and use <code>time.Parse</code>in your main to parse the date instead.</p> <p>Also notice that in Go we use camelCase so you should change variables like <code>final_time</code> to <code>finalTime</code> or maybe to something even shorter.</p> <p>By combining these suggestions, I changed the code to this: <a href="https://play.golang.org/p/XYYrFjKPEJ" rel="nofollow">https://play.golang.org/p/XYYrFjKPEJ</a></p> <p>You can probably make this code even better (for example by using <code>time.Duration</code>) but it looks fine for now.</p></pre>PsyWolf: <pre><p>I avoid recommending naked returns to new gophers. Even the go tour advises against overusing them at <a href="https://tour.golang.org/basics/7" rel="nofollow">https://tour.golang.org/basics/7</a></p> <p>But a big +1 on using time.Parse if you can.</p></pre>shovelpost: <pre><blockquote> <p>I avoid recommending naked returns to new gophers. Even the go tour advises against overusing them</p> </blockquote> <p>I agree. I made an edit on my reply to emphasize this.</p></pre>PsyWolf: <pre><p>This may be out of scope for the original question, but by combining the suggestions from my other comment with <a href="/u/shovelpost" rel="nofollow">/u/shovelpost</a>&#39;s suggestions to use time.Parse and time.Duration, you end up with damn sexy code. <a href="https://play.golang.org/p/2QZT0lrtLy" rel="nofollow">https://play.golang.org/p/2QZT0lrtLy</a> (pasted below)</p> <pre><code>package main import ( &#34;fmt&#34; &#34;time&#34; ) // reading final date from countdown file and calculating the amount // of days, hours, minutes, seconds between now and chosen final date func main() { // a string like this is read from file // day.month.year data := &#34;1.1.2020&#34; fmt.Println(&#34;Days Hours Minutes Seconds&#34;) td := TimeDiff{rowToTime(data).Sub(time.Now())} fmt.Printf(&#34;%4v %5v %6v %8v&#34;, pad(td.Days()), pad(td.Hours()), pad(td.Minutes()), pad(td.Seconds())) } // prefix number with 0 func pad(number int) string { return fmt.Sprintf(&#34;%02v&#34;, number) } func rowToTime(data string) time.Time { //see https://golang.org/pkg/time/#example_Time_Format //for more info on time format strings t, err := time.Parse(&#34;2.1.2006&#34;, data) fmt.Println(t) if err != nil { panic(err) } return t } type TimeDiff struct { diff time.Duration } func (td TimeDiff) Days() int { return int(td.diff.Hours() / 24) } func (td TimeDiff) Hours() int { return int(int64(td.diff.Hours()) % 24) } func (td TimeDiff) Minutes() int { return int(int64(td.diff.Minutes()) % 60) } func (td TimeDiff) Seconds() int { return int(int64(td.diff.Seconds()) % 60) } </code></pre> <p>You could leave everything as int64, but after all the calculations, they make more sense as ints.</p></pre>GreatDant0n: <pre><p>I heard that Go community is welcoming to newcomers. But such thorough answers I never imagined.</p> <p><strong>Thank you all!!!</strong></p></pre>PsyWolf: <pre><p>Good questions deserve good answers :-)</p></pre>PsyWolf: <pre><p>FYI, it&#39;s much easier to get help on this sort of stuff if you make a simple, runnable example on <a href="https://play.golang.org/" rel="nofollow">https://play.golang.org/</a></p> <p>When you just copy paste code right from your project that we can&#39;t run, it makes it much harder for us to help. In your particular example, it&#39;s hard for me to even tweak it to be a runnable example because I&#39;d have to reverse-engineer the contents of scripts/COUNTDOWN.txt</p> <p>Give me something I can work with and I&#39;ll be happy to show you how I&#39;d tweak the code to not need those awkward temp variables.</p></pre>GreatDant0n: <pre><p>You are right, I tweaked the example, however if you run it, it won&#39;t produce the right results. For some reason time.Now is stopped at 2009-11-10 23:00:00 +0000 UTC.</p> <p>Anyway here it is: <a href="https://play.golang.org/p/n8HmUZOqq7" rel="nofollow">https://play.golang.org/p/n8HmUZOqq7</a></p></pre>PsyWolf: <pre><p>That&#39;s much better. Thank you!</p> <p>So the big picture advice I&#39;d give you is that in all strictly typed languages (including go), you&#39;re encouraged to <strong>convert any input to its actual type as soon as possible</strong> and then <strong>keep variables as their actual type until the last possible moment</strong>. By &#34;actual type&#34;, I mean the type they represent conceptually and not the type they happen to be at the moment.</p> <p>In simple cases like this, it means treating these values like ints for just a little longer than you did. Here&#39;s a pretty small change to your code that converts right at the end and fixes the problem. I also simplified your prefixNumber function.<br/> <a href="https://play.golang.org/p/uO8rY7D8Di" rel="nofollow">https://play.golang.org/p/uO8rY7D8Di</a></p> <p>That code will do just fine in a pinch, but in a more complex codebase I&#39;d recommend a more objected oriented approach. Make and use datatypes that represents concepts instead of just data. Notice that each row is essentially a time.Time and apply the same rule of <em>converting input to its actual types as soon as possible</em> to the rows as well. That means turning your calculate function into one that just converts a row to a time.Time. Then make a class that represents the time difference you&#39;re trying to represent. Putting it all together gives you something like<br/> <a href="https://play.golang.org/p/7MChjkR0R9" rel="nofollow">https://play.golang.org/p/7MChjkR0R9</a> (edit: swapped start/end)</p> <p>It added 10 lines of code, but the separation of concerns makes it more maintainable and reusable going forward IMO.</p> <p><em>P.S.<br/> The clock thing is just a quirk of the playground. You can read more about it at <a href="https://blog.golang.org/playground" rel="nofollow">https://blog.golang.org/playground</a> if you&#39;re really curious, but it&#39;s not important.</em></p></pre>PsyWolf: <pre><p>Btw, the real trick to not needing these temp variables is do to all your conversions at function boundaries instead of in the middle of functions. When you do this, you can re-use your variable names because each version is in its own separate function, and the variable never has 2 different types in the same scope. This tends to happen automatically when you follow the 2 rules I listed above about converting only at the edges of your program.</p> <p>Here&#39;s a really basic example that shows the broader concept.<br/> <a href="https://play.golang.org/p/dHBypPj5Ss" rel="nofollow">https://play.golang.org/p/dHBypPj5Ss</a></p></pre>forfunc: <pre><p>That is because the playground had a fixed time</p></pre>

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

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