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

Accept interfaces, return structs #1247

Open
zostay opened this issue Jan 31, 2022 · 2 comments
Open

Accept interfaces, return structs #1247

zostay opened this issue Jan 31, 2022 · 2 comments

Comments

@zostay
Copy link

zostay commented Jan 31, 2022

The various Database methods currently return interfaces rather than structs, but Golang best practice indicates that returning structs is best. Consider the following situation where I want to wrap the database with a special Pinger in cases where I can use that to help determine the health of the connection. (Unrelated, I am having some weird connectivity issues with memcached and I'm working on trying to determine why using code similar to this.)

type Pingable interface {
    Ping() error
}

type MCPinger struct {
    *server.Memcached
}

func (p *MCPinger) Ping() error {
    return p.Memcached.Client.Ping()
}

If NewMemcached() returned *server.Memcached as convention suggests it should, the code to initialize the MCPinger would read:

var p Pingable = &Pinger{server.NewMemcached(connectString)}

However, right now this code would instead read something like this:

db := server.NewMemcached(connectString)
mdb, ok := db.(*server.Memcached)
if !ok {
    panic("NewMemcached() returns a different object than expected.")
}
var p Pingable = &Pinger{mdb}

Both the memcached and redis databases are implemented this way. The fix is just to change the return type of each New*() function to return the pointer to the struct it constructs rather than the interface.

@zostay
Copy link
Author

zostay commented Jan 31, 2022

(And yes I realize I could save myself the if !ok ... panic bit if I skipped the , ok in the type conversion, but in my real code I'm not panicking, but returning an error to be logged and then panicking some place higher up the call frame stack.)

@jhaals
Copy link
Owner

jhaals commented Feb 3, 2022

Hi @zostay! Yes I'd say you're right on that point and I think it make sense to update that. Would you be willing to contribute that suggestion?

I'm currently adding my spare cycles on a reimplementation of the service in typescript and nextjs to both cleanup and make it easier for users to customise as the current react app is not that easy to extend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants