Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wiki: CodeReviewComments change #33228

Open
eslam-mahmoud opened this issue Jul 22, 2019 · 4 comments
Open

wiki: CodeReviewComments change #33228

eslam-mahmoud opened this issue Jul 22, 2019 · 4 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@eslam-mahmoud
Copy link

eslam-mahmoud commented Jul 22, 2019 •
edited

Hello,

I suggest an example to be added to explain why interface constructor should return the struct not the interface type itself

The edit to be added on CodeReviewComments #interfaces

Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types: that way, new methods can be added to implementations without requiring extensive refactoring, **and this will enable the struct to implement more than one interface and be accepted/evaluated to all of them in different functions**

The code example
https://play.golang.org/p/zZU0rFzjuEm

// DONOT DO THIS
package main
import ("io")
type Thinger interface { 
	Thing() bool 
}
type defaultThinger struct{  }
func (t defaultThinger) Thing() bool { 
	return true 
}
func (t defaultThinger) Read(p []byte) (n int, err error) {
   	return 0, nil
}  	
func NewThinger() Thinger { 
	return defaultThinger{  } 
}
func main() {
	testThinger(NewThinger())
	// will return cannot use NewThinger() (type Thinger) as type io.Reader in argument to testReader: Thinger does not implement io.Reader (missing Read method)
	// if NewThinger() return defaultThinger it will work
	testReader(NewThinger())
}
func testReader(r io.Reader) {}
func testThinger(t Thinger) {}
@ianlancetaylor
Copy link
Contributor

Honestly, that seems like a minor point to me, compared to the existing point.

CC @bcmills

@eslam-mahmoud
Copy link
Author

The point is to clarify the why with an example of what can go wrong

@bcmills
Copy link
Contributor

bcmills commented Jul 22, 2019

Implementing a second interface seems like a special case of the general problem of wanting to access methods beyond the first interface.

In particular, it would seem to suggest that you could instead return an interface that composes together the various other interfaces that you intend to implement. But that would still have the same problems as returning an interface in general: it still would not allow you to add methods beyond the original interface, and still would not allow clarifying documentation on the method implementations.

@eslam-mahmoud
Copy link
Author

I agree with you, and because I faced that case I tried to add it for everyone to be one more point listed explaining why not to return the interface

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2019
@seankhliao seankhliao added this to the Unreleased milestone Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@eslam-mahmoud @ianlancetaylor @bcmills @julieqiu @seankhliao