<p>Hi, I'm playing with Go and don't know any Go developers who can have a look at my code and give me an advice, so I decided to try and ask here. I would really appreciate any feedback.</p>
<p>The majority of my programming experience was with interpreted, object oriented-programming languages like PHP, Javascript and Python which is my primary language, at the moment. And I often catch myself doing things in Go in a similar way I do them in Python.</p>
<p>The project doesn't use channels or goroutines heavily: the main goal was to try different libraries (standard and 3rd party). I wanted to:</p>
<ul>
<li>Try to work with SQL databases without ORM libraries. Just database/sql + a database driver;</li>
<li>Find a way to manage schema migrations for a DB. BTW, probably I made a mistake: it seems the library of my choice (<a href="https://github.com/mattes/migrate" rel="nofollow">github.com/mattes/migrate</a>) is <a href="https://github.com/mattes/migrate/issues/311" rel="nofollow">going to die</a>;</li>
<li>Discover an easy way to build POSIX-compatible CLI tools: <a href="https://github.com/spf13/cobra" rel="nofollow">Cobra</a> looks great;</li>
<li>Try to write unit tests and mocks: I like the standard testing package and <a href="https://github.com/golang/mock/" rel="nofollow">github.com/golang/mock</a>;</li>
<li>Try to follow SOLID principles;</li>
<li>Not directly Go related: try GNU Make for building projects. I've noticed that a lot of open source projects (including Go projects) use Make and I realised that I've never tried it before.</li>
</ul>
<p>Code can be found on GitHub: <a href="https://github.com/m1kola/shipsterbot" rel="nofollow">https://github.com/m1kola/shipsterbot</a>. It's a stupid telegram bot that allows you to manage your shopping list (in a private chat or in a group chat).</p>
<p>Thank you for your time!</p>
<hr/>**评论：**<br/><br/>ar1819: <pre><p>Alright - first of all, I think you should read about <a href="https://dave.cheney.net/2013/10/12/how-to-use-conditional-compilation-with-the-go-build-tool" rel="nofollow">build tags</a> and <a href="https://dave.cheney.net/2014/09/28/using-build-to-switch-between-debug-and-release" rel="nofollow">how you can use them</a> to create test/debug/release binaries. The reason I'm saying this is that assigning function to a variable, because you want to override it later in your tests, may seem like a good idea at first, but complicates the "code research" later. A lot.</p>
<p>I would also suggest to read about <a href="https://github.com/alecthomas/gometalinter" rel="nofollow">gometalinter</a> - it's a very useful tool, and it will help you uncover some of the (potential) bugs.</p>
<p>Third - try not to use <code>fmt.Errorf</code> result as a direct value for return. The reason is - it's extremely hard to analyse in method\function callers, because it's just a text. Same with <code>errors.New</code>. Create a new global variables - later you will thank you. If you need to carry some sort of meta information you are not going to inspect - use <a href="https://github.com/pkg/errors" rel="nofollow">pkg/errors</a>. If you need to actually analyze error - create a new type which implements error interface.</p>
<p>Forth - do not declare interfaces if you not planning to use different implementations. Actually prefer to <a href="https://medium.com/@cep21/what-accept-interfaces-return-structs-means-in-go-2fe879e25ee8" rel="nofollow">accept interfaces</a>, <a href="http://idiomaticgo.com/post/best-practice/accept-interfaces-return-structs/" rel="nofollow">return structs</a> if you can. It works wonderfully in Go, and helps with reasoning about dependencies on sight.</p>
<p>Fifth - read about <a href="https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis" rel="nofollow">functional options</a>. It a very useful pattern in Go, when you want to have optional configuration for your objects.</p>
<p>Sixth - prefer integration to unit-tests. I know it may sound weird, but too much of your code is overrided in test files, so you are essentially testing tests and not the app code. Or try to minimize the number of mocks. IMHO of-course.</p>
<p>Seventh - read about <a href="https://github.com/golang-standards/project-layout" rel="nofollow">common go</a> <a href="https://medium.com/golang-learn/go-project-layout-e5213cdcfaa2" rel="nofollow">project layouts</a>. It's not enforced, and everyone can develop their own layout - but I really like when code is separated from migration files and assets.</p>
<p>Overall it's a very nice piece of software, and you approach is very solid (pun unintended). The only thing I didn't like, is that such small app left me with "Java feel". While I agree that following SOLID is important - YAGNI and KISS are equally important.</p>
<p>P.S. You might want to fix</p>
<blockquote>
<p>hideInlineKeybaord</p>
</blockquote>
<p>this =)</p></pre>m1kolka: <pre><p>Thank you very much for your comprehensive and useful feedback.</p>
<blockquote>
<p>The reason I'm saying this is that assigning function to a variable, because you want to override it later in your tests, may seem like a good idea at first, but complicates the "code research" later. A lot.</p>
</blockquote>
<p>From the very beginning I had a feeling that I'm not doing the right thing, for several reasons: it's quite easy to override a function and forget to restore it, for example... Also it doesn't seem to be right semantically: I'm not changing the variable in the release build. But I didn't manage to find a good way to mock functions.</p>
<p>I've just read these two blogs posts. Using build tags for this sounds like a good idea, but I'm not sure that it's clear to me how to mock functions using build tags. Do I understand correctly that you are hinting at something like this?</p>
<ul>
<li>Make all functions to be functions (without assigning to a variable);</li>
<li>Move all functions I need to mock into separate files and build them only when not running tests (by introducing a build tag);</li>
<li>Define alternative implementations for tests. I guess for tests I still need to assign functions to variables, because some functions should return different values in different test cases;</li>
</ul>
<blockquote>
<p>try not to use <code>fmt.Errorf</code> result as a direct value for return. The reason is - it's extremely hard to analyse in method\function callers, because it's just a text. Same with <code>errors.New</code>. Create a new global variables - later you will thank you.</p>
</blockquote>
<p>I have a custom error type <a href="https://github.com/m1kola/shipsterbot/blob/49994438cc64f07353ea7af5fd45f5a122c8ec92/bot/telegram/errors.go#L5-L11" rel="nofollow">here</a> and a global error variable for a specific case, because I need to understand difference between the following types of errors:</p>
<ul>
<li>Something wrong with the infrastructure. For example, a storage (DB) is not not accessible. In this case I want to notify an user ("Sorry, something is wrong. Please, come back later"-like message);</li>
<li>User sends non-sense. Then the bot needs to reply like "Sorry, I don't understand you. I support the following commands: ...";</li>
<li>I tried to route an update I received from Telegram to a specific group of handlers, but didn't found a match, so I need to try to route this to another group of handlers;</li>
<li>As an edge case of previous one: I tried to route the update, and found an appropriate group of handlers, but it looks like user sends something that we cant handle (unknown command, for example).</li>
</ul>
<p>Does it make sense?</p>
<p>I use <code>fmt.Errorf</code> and <code>errors.New</code> only when I know that function callers do not care about this error much. But it's different in tests. If a function returns two errors created by <code>fmt.Errorf</code> or <code>errors.New</code>, I might want to check that I've got a specific error. Currently in few tests I'm asserting error text (using <code>strings.Contains</code>) which is stupid, I know. But I'm not sure that I want to define a global variable only for test assertions. Also I'm not sure that I actually need to assert specific errors when I know that function callers do not care about specific type of error. Probably I should just check that the function I test returns <code>error</code>, not <code>nil</code>.</p>
<blockquote>
<p>Sixth - prefer integration to unit-tests. I know it may sound weird, but too much of your code is overrided in test files, so you are essentially testing tests and not the app code. Or try to minimize the number of mocks. IMHO of-course.</p>
</blockquote>
<p>I don't want to have a database to be able to test my app code, that's why I use unit tests and mocks rather than integration tests. I'll, probably, add end-to-end tests to ensure that whole workflow works as expected and will run them only on CI.</p>
<blockquote>
<p>so you are essentially testing tests and not the app code</p>
</blockquote>
<p>Hm. I can't find places where I'm testing tests... If you see something like that in my tests, please, send me a link. I often have cases when a function <code>foo</code> calls a function <code>bar</code>. I override whole function <code>bar</code> when I'm testing <code>foo</code> to return a specific result and test <code>bar</code> separately. So I know that <code>bar</code> behaves in the way I expect and <code>foo</code> behaves in the way I expect. But I can't find places where I'm testing tests.</p>
<blockquote>
<p>P.S. You might want to fix</p>
</blockquote>
<p>Good catch! :)</p>
<p>Thank you for your feedback and a lot of useful links. This is very helpful. Now I have a lot to read and try :)</p></pre>ar1819: <pre><p>With build tags, you can define one implementation which is actually accessштп fs\network\whatever and other, test, implementation, which internals (for example - expose global variables) you can override in your test files. Just define one starting with:
<code>// +build !my_test_tag</code><br/>
and the other, testing one, with<br/>
<code>// +build my_test_tag</code></p></pre>
