Why golint treats this as warning?
jeevatkm opened this issue · 16 comments
It's common to use this pattern/structure in go.
type sample struct {
// fields goes here
}
fun NewSample(/* input variables */) *sample {
// specific initialize logic goes here
// this package doesn't export sample struct directly
return &s
}golint warns "exported func NewSample returns unexported type *main.sample, which can be annoying to use (golint)"
It's not a common pattern because callers of that function from other packages will struggle to use it. For instance, they can't put a *sample field in their own struct.
I agree with they can't put a *sample field in their own struct. Thanks, will update my implementation.
I have a code which looks like below:-
type myMonitor struct {
isEnabled bool
}
var monitorObj *myMonitor // singleton object
// method to ensure singleton behaviour
func GetInstance() *myMonitor {
if monitorObj == nil {
monitorObj = new(myMonitor)
}
return monitorObj
}
func (obj *myMonitor) Hello {
...
}golint is reporting warning as:-
warning: exported func GetInstance returns unexported type *monitor.myMonitor, which can be annoying to use (golint)
Should golint report this as warning?
The code snippet you presented is still somewhat strange. Since myMonitor is not exported, it will not be shown in godoc, so the user will not even know about the existence of the myMonitor.Hello method, even if they are able to call it.
Thus, I believe lint is correct in complaining that GetInstance is returning an unexported type.
I think that is the way to implement singleton as mentioned here
Contrary to what that blog recommends, I think it's a poor idea to make the type of the singleton unexported (again, for the reason that the documentation will not show exported methods on an unexported type).
I would just make myMonitor exported. What is your fear with doing that?
If I make myMonitor as exported then anyone will be able to instantiate it any number of times violating the singleton constraint.
That is a possibility, but you can document it that people should not do as such.
If you want to add the additional boilerplate, you can define an interface:
func GetInstance() Instance { ... }
type Instance interface {
Hello()
Goodbye()
private() // This private method prevents others from implementing this interface
}This has the advantage that the methods of the singleton are clearly documented. The reason we want to make sure that the interface cannot be implemented is so that you have the future flexibility of adding more methods to the singleton. If other people can implement the interface, you are barred from ever altering the interface.
There is the valid use-case that an unexperted struct implements multiple interfaces, e.g. Reader and Writer. If this pattern is forbidden, we end up defining union interfaces like ReaderWriter. This is ugly.
@sttts can you elaborate, for example with a concrete example?
If you're suggesting that this
func constructor() *someType { ... }
followed by the client type-asserting o Reader or Writer
is better than this
func constructor() ReaderWriter { ... }
then I have to disagree.
constructor must be Constructor for it to make sense.
ReaderWriter is a bad example as they are concepts that appear as a pair often. Instead, something like Callback, Reader, Order, all completely unrelated, but implemented by *someType. I don't want CallbackReaderOrder as a generic interface outside of the package for obvious reasons. So maybe the best solution is to define a SomeType interface, along the lines of https://play.golang.org/p/Hf95uVxCML. Wdyt?
Yes, that would be the canonical approach, if you can't export the concrete type.
A local super interface doesn't work unfortunately:
type A interface { Foo() }
type B interface { Foo() }
type C interface {
A
B
}This fails because Foo() is defined twice in C. So we are left with a public struct and private fields.
package waterbottle
type WaterBottle interface {
Liters() int
}
func NewWaterBottle() WaterBottle {
return &plasticWaterBottle{liters: 1}
}
struct plasticWaterBottle type {
liters int
}
func (b *plasticWaterBottle) Liters() int {
return b.liters
}This is illegal because plasticWaterBottle is unexported.
That's unfortunate: we are trying to encapsulate implementation behind an interface.
A client has no business working with plasticWaterBottle. They only need a WaterBottle.
What about this case? Look at these packages:
package user
type storage struct {
db *sql.DB
}
func New(db *sql.DB) *storage {
return &storage{db: db}
}
func (s *storage) Set(id int) error { … }
package expuser
type Storage struct {
db *sql.DB
}
func New(db *sql.DB) *Storage {
return &storage{db: db}
}
func (s *Storage) Set(id int) error { … }
Lets try to use it (in non-proper way):
package main
import "user"
func main() {
store := user.storage{}
store.Set(1)
}
// compile time error: cannot refer to unexported name user.storage
And another one:
package main
import "expuser"
func main() {
store := expuser.Storage{}
store.Set(1)
}
// panic: runtime error: invalid memory address or nil pointer dereference
I guess, compile time errors are better than runtime errors.
It's called encapsulation and I can't understand why it's bad?