[wiki] CodeReviewComments: add "Embed Struct Fields" and "Don't Use Interfaces For Function-Only Types"

[matthe...@gmail.com]

Hello,

I’d like to add these two sections to https://github.com/golang/go/wiki/CodeReviewComments based on experience giving golang-nuts code reviews in the past two months. The page says discuss even minor changes, so any feedback is appreciated.

Embed Struct Fields

Struct embedding can simplify code throughout a project.

Instead of

type species struct {
    name  string
    genus string
}

type quacker interface {
    Quack() string
}

type duck struct {
    mu *sync.Mutex
    q  quacker
    s  species
}

// called like v.mu.Lock(), v.q.Quack(), v.s.name, and v.s.genus

embed the fields to simplify access.

type duck struct {
    *sync.Mutex
    quacker
    species
}

// called like v.Lock(), v.Quack(), v.species, and v.genus

The lack of stuttering struct access improves readability of Go code.

A caveat is that the methods and fields of embedded types are promoted to the struct type which may be unwanted in library design or for godoc clarity.

Don’t Use Interfaces For Function-Only Types

When an interchangeable set of functions without additional type data is necessary use the ability to include functions in structs to define a type where the data is the functions.

// an interface groups behavior which makes them tempting for this problem
type Tool interface {
    Load(string) error
    Execute(string) error
}

// this approach requires adding excess types
type GoTool struct{}

type (a GoTool) Load(path string) error {…}
type (a GoTool) Execute(args string) error {…}

type CppTool struct{}

// these methods may do nothing with a
type (a CppTool) Load(path string) error {…}
type (a CppTool) Execute(args string) error {…}

Instead of using an interface type use a struct type of functions.

type Tool struct {
    Load    func(path string) error
    Execute func(args string) error
}

var (
    GoTool = Tool{
        Load:    func(path string) error {…},
        Execute: func(args string) error {…},
    }
    CppTool = Tool{
        Load:    func(path string) error {…},
        Execute: func(args string) error {…},
    }
)

Matt

[Ian Lance Taylor]

Personally I'm not sure I agree with this recommendation. An embedded
field can be useful in specific circumstances, such as embedding a
sync.Mutex (contrary to your example, I don't think I would ever
recommend embedding a *sync.Mutex). It can also be useful when types
have a strict is-a relationship. I don't grasp the use of quacker in
your example; I would typically expect duck to have a Quack method and
thus to implement the quacker interface, I wouldn't ordinary expect
duck to embed a Quacker field.

This can sometimes be appropriate advice but I'm not sure it rises to
the level of a style guideline. It's more like a practice to consider
using when appropriate. Or so it seems to me.

Ian

[Peter Weinberger]

Another consideration in embedding is exporting.

type Foo {

 mu sync.Mutex

 ...

}

has mu as part of the implementation

type Foo {

 sync.Mutex

...

}

makes Foo.Lock and Foo.Unlock public, and seems mostly undesirable.

Ian

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[matthe...@gmail.com]

I don't grasp the use of quacker in 
your example; I would typically expect duck to have a Quack method and 
thus to implement the quacker interface, I wouldn't ordinary expect 
duck to embed a Quacker field. 

The application would be where a quacker implementer is also part of things other than ducks and not every duck has the same quacker.

The duck type does implement the quacker interface by having the embedded quacker field (https://play.golang.org/p/GIJmoS_UXha). A duck shouldn’t have a duck as its quacker; if there was a single flock representing duck then the individually assigned quackers should be the representing duck's quacker, not the whole duck. But functions could take a duck as a quacker var.

(contrary to your example, I don't think I would ever recommend embedding a *sync.Mutex)

Can you explain your view on this? I embed Mutex pointers to pass structs by value instead of by pointer while also having the embedding shortcut.

This can sometimes be appropriate advice but I'm not sure it rises to the level of a style guideline.

It was something I saw in the code reviews, in production code (one of the github.com/gorilla libs), and in playground examples during discussion. “type Something struct{}” is a code smell to me.

Another consideration in embedding is exporting.

I did include this as the caveat but the term “embedding is exporting” is clearer.

Thanks,

Matt

To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

[Ian Davis]

On Mon, 5 Feb 2018, at 3:41 PM, matthe...@gmail.com wrote:

(contrary to your example, I don't think I would ever recommend embedding a *sync.Mutex)

Can you explain your view on this? I embed Mutex pointers to pass structs by value instead of by pointer while also having the embedding shortcut.

Personally, I avoid it for a couple of reasons:

1) If you do this then you could have two copies of a value that share a mutex. Locking one would affect the other.

2) the zero value of the struct is not usable since the lock is nil

[matthe...@gmail.com]

Specifically the structs with *sync.Mutex contain read-only data and shared data pointers. There's a New function with make and &sync.Mutex{}. Maybe the embedded *sync.Mutex is too specific for a general embedding example.

Matt

[Bryan C. Mills]

On Sun, Feb 4, 2018 at 11:14 AM, <matthe...@gmail.com> wrote:

Hello,

I’d like to add these two sections to https://github.com/golang/go/wiki/CodeReviewComments based on experience giving golang-nuts code reviews in the past two months. The page says discuss even minor changes, so any feedback is appreciated.

Embed Struct Fields

Struct embedding can simplify code throughout a project.

Instead of

type species struct {
    name  string
    genus string
}

type quacker interface {
    Quack() string
}

type duck struct {
    mu *sync.Mutex
    q  quacker
    s  species
}

// called like v.mu.Lock(), v.q.Quack(), v.s.name, and v.s.genus

embed the fields to simplify access.

type duck struct {
    *sync.Mutex
    quacker
    species
}

// called like v.Lock(), v.Quack(), v.species, and v.genus

The lack of stuttering struct access improves readability of Go code.

A caveat is that the methods and fields of embedded types are promoted to the struct type which may be unwanted in library design or for godoc clarity.

See the discussion on https://golang.org/issue/22013, which includes several pitfalls of embedding.

In this example, duck should embed species only if every behavior of a species (including methods added in the future!) should be observable in a duck.

That's not at all obvious to me here, so I would avoid embedding in this case (and would discourage it in a code review).

See https://golang.org/issue/21670 (and the ensuing discussion about generalized “interface literals”).

[matthe...@gmail.com]

See https://golang.org/issue/21670 (and the ensuing discussion about generalized “interface literals”).

I added a comment.

In this example, duck should embed species only if every behavior of a species (including methods added in the future!) should be observable in a duck.
That's not at all obvious to me here, so I would avoid embedding in this case (and would discourage it in a code review).

Saying a duck is a species and has all information and behavior of a specific species makes sense to me. Can you explain your view more?

See the discussion on https://golang.org/issue/22013, which includes several pitfalls of embedding.

I’ll read this in detail. Previously I did a quick pass-through and left examples from my code.

I’ve used struct embedding for is-a, has-a, and both, but the access simplifications while keeping marshaling compliance is why I suggest it. A Go 2 thought is allowing embedded fields to not have their methods exported to godoc somehow.

Thanks,

Matt

[matthe...@gmail.com]

Based on the response here I’ll leave the wiki unchanged. I would relabel these proposed sections “Consider Struct Embedding” and “Don’t Overuse Interface” now, but I’ll come back with a new/updated proposal if appropriate after gaining more experience. I’ll continue applying these plus the added knowledge in this thread to code reviews.

Thanks all for your feedback.

Matt