In Golang, is it generally preferred to write methods that return new values and then save the result, or methods that mutate receivers and arguments?

I'm totally new to Go, and I was hoping someone could give me a bit of guidance as to what the best practices/patterns in it are.

Say that I'm writing a tool that connects to public APIs over the internet, and saves response information to a local database. I make a struct, apiClient, and a constructor function, newApiClient(), that initialises a fresh one for me -- it authenticates and gets my access token, sets up a DB connection, starts a timer so I know when my token expires, and so on.

Now, setting up a DB connection is a fair chunk of code, what with the error handling and such. So I make newDbConnection() its own function.

Here's my question.

Is it generally better to make newDbConnection() a pointer-method (as in, func (client *apiClient) newDbConnection()) that creates a DB connection and then mutates the apiClient's dbConn field, or to make it a plain old function that just returns a new DB connection, and then call apiClient{dbConn: newDbConnection()} in the constructor?

In most languages, I would prefer the latter, because there's always a preference towards functions as close to pure as you can get them. But I usually work in functional languages with immutable structures, where everything you pass or return is invisibly a reference and it doesn't matter because you can never change values. In Go's case, I'm more uncertain: is it more efficient to directly mutate with pointers rather than deal with assignment and return and if so, is the benefit of this generally considered worth the reduced readability? (Or is it actually MORE readable for Go programmers since they're used to mutation existing?)

---

评论：

elagergren:

Well, first—just to make sure this isn't an A/B problem—make sure you check out the database/sql and database/sql/driver packages.

Second, I'd prefer the latter. However, if this is all local to your NewAPIClient function, it doesn't really matter—do whatever is the easiest to maintain and expresses what you're trying to do the best.

0xjnml:

nit: mutating the receiver/function argument is not observable at the call site.

aboukirev:

There are a few things to consider in addition to what you already know.  First, consider memory allocations: the fewer you make, the easier it is on the garbage collector and fewer possibilities for memory leaks.  Second, opening database connection returns a reference to a pool, which will handle multiple actual connections for you. Maybe you do not need to create new connections yourself (unless you are opening one per goroutine).    

skidooer:

If the API client depends on a database handle, pass it in on construction:func newApiClient(db *sql.DB).

But, more importantly, does an API client that connects to public APIs over the internet really depend on a local database connection? I get the impression you are mixing concerns. You may want to rethink your architecture completely (or, at least your naming if I have been given the wrong idea of what the structure is doing).

akavel:

In the case you described, the "functional-like" approach would be preferable. As a general rule, aiming for readability is highly valuable and "good style" in Go too. (By the way: if you choose the opposite pattern in the end, it's preferable to not call it newSomething, which by convention is used for functions returning the created object. I tend to use initSomething in the rare case I pick this approach; or possibly resetSomething, or else, depending on exact semantics.)

Regarding what is "more efficient"... in general, it's advised to "measure first[, and only then maybe optimize based on that]". Technically, testing.Benchmark helps here (there's also some tool to compare results of Benchmark before vs. after code changes, but I don't recall where to find it). It may well show up that compiler would actually optimize both patterns to same code! Anyway, one should also be looking at "measure" in wider perspective, not just of a microbenchmark, but of a whole program. Specifically: is your function going to be ever called in a hot loop/hot path of some code? Or maybe the connections will be pooled, so small performance differences don't really matter much? Even in case of a hot path, won't later calls of the actual SQL queries take long enough to make performance of the constructor irrelevant in comparison anyway?

But please note that this comment is specifically aimed at the particular case you asked. If I were to look only at the one-line header/subject of your question, without reading the details, I'd actually answer quite differently!

As a closing note, it's especially worthwhile to read the standard library code from time to time, to get some feeling for what's generally observed as "good style". (Though with some caveats in mind, too; e.g. usage of assembly wouldn't generally be a good idea outside of stdlib.)

zandorz:

http://tmrts.com/go-patterns/

adamtanner:

The latter is more common.

It is easier to test (sql.Open a local/in-memory/mock DB in test and give that to the client) and allows you to reuse the same sql.DB instance which is generally a good idea because it actually represents a pool of connections and is safe for concurrent use.

EDIT: just realized you said you were doing the dbConnection() call in the constructor. It's often a better idea to pass the handle to the database as an argument to the constructor and do the sql.Open call in main or your test. Generally makes the whole system easier to test and reduces coupling.