<p>Even though I read a lot about Go and best practices, I have barely written any Go code yet. So currently I am working on a small project but already have some questions regarding code design.</p>
<p>I read about passing interfaces being preferred over passing explicit values to make code more testable, reusable and expressive.</p>
<p>Here is a concrete example to explain my questions:</p>
<p>I want to read a configuration file. Therefore, I created a file <code>config.go</code> that has three functions: <code>read(filePath string) (r io.ReadCloser, err error)</code>, <code>parse(r io.ReadCloser) (c Config, err error)</code> and <code>LoadConfig(file string) (c Config)</code>.
To make testing easier, I tried to pass interfaces where possible.</p>
<p>These are my questions:</p>
<ol>
<li>Is it good design to split functions like that? I am mainly thinking about testability here, for example to test parsing without having to a file.</li>
<li>Is there a way to avoid having to pass an <code>io.ReadCloser</code> when working with files like this? The problem I faced was that by the time I wanted to decode the file in <code>parse()</code> it had already been closed by <code>read()</code> (using <code>defer file.Close()</code>) so I moved the responsibility of closing to <code>parse()</code> but that feels weird.</li>
<li>Is passing interfaces considered the general best solution in Go? The reason why I am asking is because to me it feels like doing this creates a shift in responsibilities in the code. As an (unrelated) example think of a function that calls another function to open a file and return its contents. But because that function takes an <code>io.Reader</code> instead of the actual <code>string</code> filename as arguments, the calling function now has to open the file and pass a <code>Reader</code>, where all it wanted was to call a function and get filecontents.</li>
</ol>
<p>I am very thankful for any clarification on this or hints in the right direction to read up on this topic.
Have a great day :)</p>
<hr/>**评论：**<br/><br/>rangeCheck: <pre><p>A few thoughts:</p>
<ol>
<li><p>Why do you need <code>read(filePath string) (r io.ReadCloser, err error)</code>? What's the difference between this function and, say, <code>os.Open</code>?</p></li>
<li><p>Why do you take <code>io.ReadCloser</code> instead of <code>io.Reader</code> in <code>parse</code> function? I don't think you really want to handle the closing in <code>parse</code> function (it's not its job). I'd prefer passing in <code>io.Reader</code>, and close the closer (if it's really a closer) in the caller.</p></li>
</ol>
<blockquote>
<p>Is passing interfaces considered the general best solution in Go</p>
</blockquote>
<p>Yes.</p></pre>evantreb0rn: <pre><ol>
<li>Actually all that function does is call <code>os.Open</code>. The reason being that I wanted <code>config.go</code> to be completely independent (pass a path, get a config). This was also the basic thought for my 3rd question: With accepting interfaces, there is always a responsibility passed to the caller (e.g opening a file to pass an io.Reader). To me this feels like a strangely tight coupling. I think that seperate modules should not share any responsibilities like that. But I am new to Go and still learning so ¯\_(ツ)_/¯ </li>
<li>The reason for that is that I used to <code>defer file.Close()</code> in <code>read()</code> but this obviously closes the file before it can be read in <code>parse()</code>. I will probably go with <a href="/u/the_jester" rel="nofollow">u/the_jester</a>'s idea and pass a Buffer instead so I can close the file in <code>read()</code> already.</li>
</ol></pre>rangeCheck: <pre><ol>
<li>Why can't your <code>LoadConfig</code> function call <code>os.Open</code> instead of <code>read</code>? I still can't see what benefits wrapping <code>os.Open</code> into <code>read</code> brings us.</li>
<li>No you don't want to read the whole file into <code>bytes.Buffer</code>, that's waste of memory. Your <code>LoadConfig</code> function could be something like (assuming <code>parse</code> takes <code>io.Reader</code> instead of <code>io.ReadCloser</code>):</li>
</ol>
<p><code>
func LoadConfig(path string) (Config, error) {
f, err := os.Open(path)
if err != nil {
return nil, err
}
defer f.Close()
return parse(f)
}
</code></p></pre>evantreb0rn: <pre><p>You are right, that is actually very much cleaner! Thanks for your help :)</p></pre>jrkkrj1: <pre><p>You seem to be down the right path. The fact that read and parse aren't exported means that awkwardness of doing the close in a separate function is OK because you aren't leaking that to the consumer. You could refactor it 100 times without a consumer knowing. That means you can live with the awkwardness for now and fix it later.</p>
<p>In terms of interfaces, just consider them to be dependency injection points. Your module to parse a config depends on something to parse from (aka the ReadCloser). The read function may be better as an "open" more than read since you could get a ReadCloser from the operation and aren't reading until you parse it.</p>
<p>My 2 scents. Also, for potential inspiration, maybe some code in github.com/spf13/viper might help?</p></pre>evantreb0rn: <pre><p>Thanks a lot for your feedback! I was actually watching a talk with Steve Francia (spf13) to understand passing interfaces better :)</p></pre>the_jester: <pre><p>Depending on assumptions you're willing to make about the config file, you can definitely pass just an io.Reader into parse. As you say, giving the parse function responsibility for closing a file feels awkward. All parse "should" have to do is. . .parse.</p>
<p>One way to do that is: read the entire file into a bytes.Buffer in your "read" function, and close the file. Define your "parse" function to accept an io.Reader, and then pass the buffer full of its contents(instead of the file) into the parse function.</p>
<p>Note this also fixes your testing problem, now you can just pass strings into "parse" for testing.</p>
<p>Defining functions which accept interfaces is very good practice. Remember that anything which <em>implements</em> that interface can then be passed in. By accepting an interface argument you are usefully restricting yourself. It makes you define responsibilities more carefully, exactly as you're working on doing.</p></pre>evantreb0rn: <pre><p>Thanks a lot for your feedback, I like the idea of passing a Buffer instead very much and will do it that way. That is way cleaner!
With regards to testing, how would I test that read function? I do not see another way than either actually creating a temporary file and deleting it afterwards or leaving the code untested :/</p></pre>the_jester: <pre><p>I'd say testing it is low priority, as you'd basically be testing Go's ability to read a file (which usually works).</p>
<p>However, if you do want to test it, you can just make various pre-made test file(s) you run through it. Not too big a deal.</p></pre>jimbishopp: <pre><p>io.ReadCloser is the only interface you should need. A bytes.Buffer is a io.ReadCloser and can be used as a config file for testing.</p>
<ol>
<li><p>Splitting control flow can be beneficial for reuse, testing, and readability; but it can also make things more fragmented. Start by keeping everything in a single routine to gain a feel for what is needed to complete the flow, and only break things up once the benefit is obvious (doesn't introduce much more complexity).</p></li>
<li><p>The weird feeling is a code smell. Try not to pass the io.ReadCloser around because it introduces uncertainty about when it will be closed and what routine is responsible for doing so (as you point out). Keep open/close flow in the same function to take advantage of defer and reduce the potential for defects.</p></li>
<li><p>Interfaces are an abstraction ((abstractions add complexity (limit complexity)). Packages, such as io, often <em>accept</em> interfaces as parameters in <em>exported</em> functions as a means of allowing the caller to pass their own concrete implementation. Using interfaces in the un-exported portions of a package is typically limited to describing a concrete structure from another package for testing; e.g. accept a SQLQuery interface that defines database/sql.DB.Query(). So it is the <em>calling</em> code that implements the interface, not the package where the concrete implementation is defined (sql). Start with a concrete implementation and refactor to an interface when the trade-off is beneficial.</p></li>
</ol>
<p>In your specific case, I suggest putting everything in a single function and use an encoding package with ioutil. You didn't mention how the configuration was encoded, but assuming it is JSON, you can read with ioutil.ReadAll, close using defer, and use encoding/json.Unmarshal to decode to a Config. This will result in a few lines of concise and defect free code that is so severely underwhelming you will not want to split it into multiple functions.</p>
<p>Typically, the standard library has the ability to make things dead simple while also being a great place to review how interfaces are used along with other idioms for writing in Go.</p></pre>evantreb0rn: <pre><p>Thanks a lot for your feedback as well!
One reason I split the flow up was to help testability. Keeping the code in one single function surely makes it harder to test (classic example of having to create a file for testing vs. passing a type that implements io.Reader for example). I agree that this function would be underwhelmingly short but therefore not easily testable. Would functions like those (with trivial flows) generally go untested? </p></pre>jimbishopp: <pre><p>Changing <code>LoadConfig(string) Config</code> to <code>LoadConfig(io.ReadCloser) Config</code> would solve your testing concern. </p>
<p>Testing short routines that only compose standard library calls is up to you. Some folks are weird about 100% code coverage. I prefer not to retest the standard library and instead move on to something more productive.</p></pre>evantreb0rn: <pre><blockquote>
<p>I prefer not to retest the standard library
That was exactly my thought as well. Thanks for reassuring me in this.</p>
<p>Changing LoadConfig(string) Config to LoadConfig(io.ReadCloser) Config would solve your testing concern.
I talked about this in another thread already (and it's also the reason for my 3rd question). If I did this, I would move responsibility to open the file to the caller, which I think is not the best way to do it as I wanted to separate concerns.
Thinking about it now it is maybe actually not so bad. I will give this a try :)
Thanks a lot for your help!</p>
</blockquote></pre>
