2
\$\begingroup\$

I'm working on a new project with go. So I figured I would give this new feature a run (it's fun!). Although, I'm not sure I'm implementing this the way it's suposed to be.

I've read the following guides before writing my code :

Golang official generics tutorial Another nice and complete tutorial by bitfieldconsulting For reference this is a simple package where I connect to mongo and get/create/update documents.

Please note the switch part in the following code. Casting it into an empty interface to be able to get the struct type seems way too hacky and not the "golang way". I know for now they didn't add any support to get the type from a generic. So this is what I did to make it work.

// I use an interface to constraint the structs types I want to work with
type Restrains interface {
    demande.Patient | demande.DemandTransport | demande.Configuration | demande.Prestataire | demande.Team
}

func setConnection[T Restrains](db *mongo.Client, object T) *mongo.Collection {
    var coll *mongo.Collection
//This is where it gets "hacky" and I don't really like it
    switch (interface{})(object).(type) {
    case demande.Patient:
        coll = db.Database(databaseName).Collection(patientCollection)
    case demande.DemandTransport:
        coll = db.Database(databaseName).Collection(demandeCollection)
    case demande.Configuration:
        coll = db.Database(databaseName).Collection(configurationCollection)
    case demande.Prestataire:
        coll = db.Database(databaseName).Collection(prestatairesCollection)
    case demande.Team:
        coll = db.Database(databaseName).Collection(teamsCollection)
    default:
        return nil
    }
    return coll
}

//Here is a simple, nice Save Object T function
//I use a pointer to be able to return nil in case of error, so I don't need to search for the type. (Not sure if this is good practice either)
func SaveObject[T Restrains](db *mongo.Client, object T) (*T, error) {
    coll := setConnection(db, object)

    obj, err := coll.InsertOne(context.TODO(), object)
    if err != nil {
        log.Error(err)
        return nil, err
    }
    obj.InsertedID.(primitive.ObjectID).String()
    return &object, nil
}

//This is a function where we get a single obj by ID from mongo
// GetOneObject needs an empty object to know which type of object to get (is this clean?)
func GetOneObject[T Restrains](db *mongo.Client, id string, object T) (T, error) {
    var foundobj T
    chs := setConnection(db, object)

    did, errId := primitive.ObjectIDFromHex(id)
    if errId != nil {
        log.Error(errId)
        return foundobj, errId
    }
    filter := bson.M{"_id": did}

    findErr := chs.FindOne(context.Background(), filter).Decode(&foundobj)
    if findErr != nil {
        if findErr != mongo.ErrNoDocuments {
            log.Error(findErr)
            return foundobj, findErr
        }
    }

    return foundobj, nil
}

As you can see, I'm questioning this is a good method. Either I keep my usual and well-known workflow for this kind of implementation, or I go down this road. I don't find it particularly difficult to maintain/read, but it might my my lack of experience with go, and/or generics. Any lights on this would be appreciated.

I understand that mongo (the official driver) handles already interfaces, but I like the idea of using constraints to keep it clear.

\$\endgroup\$
1
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Apr 13, 2022 at 5:44

1 Answer 1

2
\$\begingroup\$

OK, so your setConnection implementation just looks to me to be using generics for the sake of using them. Functionally, there would be no difference between your function being written as either

func setConnection[T any](db *mongo.Client, object T) *mongo.Collection

or simply

func setConnection(db *mongo.Client, object interface{}) *mongo.Collection

The setConnection function is not exported, and called by functions that all have the [T Restraints] constraint already, so whatever is passed to setConnection is pretty much guaranteed to work already. What you have now, as you already know, is a bit of a hack. Clearly, each type has a corresponding collection variable associated with it already. You're using a type assertion to work out what variable you want to use. That's something was quite easily done before generics were introduced to the language already:

type Collection interface {
    CollectionName() string
}
// then in your object types:
package demande
func (_ Patient) CollectionName() string {
    return patientCollection // a const or something
}

Then, in your setConnection function, you'd simply write:

func setConnection(db *mongo.Client, object Collection) *mongo.Collection {
    return db.Database(dbName).Collection(object.CollectionName())
}

Nice and easy.

If the collection argument isn't something you can have in the code as a const, or you can't/don't want to have it anywhere but in the package where you interact with mongo, you can just keep the same interface, and use a simple map:

package mongostuff

var collections = map[string]collectionType{
    demande.PatientName: patientCollection, // and so on
}

and your setConnection function would then look something like this:

func setConnection(db *mongo.Client, object Collection) *mongo.Collection {
    return db.Database(dbName).Collection(collections[object.CollectionName()])
}

If you don't even want to bother going through the interface/constant route, you could (but really shouldn't) go and use the reflection package:

var collections = map[reflect.Type]CollectionT{
    reflect.TypeOf(demande.Patient{}): patientCollection,
}

In which case setConnection turns in to this:

func setConnection(db *mongo.Client, object interface{}) *mongo.Collection {
    return db.Database(dbName).Collection(collections[reflect.TypeOf(object)])
}

The type of the object argument could be whatever makes most sense overall. You're using reflection though, which is bad, so once again: don't use this. Much like generics: you shouldn't use reflection just because you can. You should only use it if you can't use anything else.

As for your other functions, I'd simply remove the word object from them, and just call them Save and GetOne, and instead of passing in object T, I'd probably use object *T so the caller knows not to mess with the objects that are being passed in, and you avoid code like this:

obj := demande.Patient{} // initialise this
// some code
savedObj, err := dbpkg.SaveObject(db, obj)
if err != nil {
    // handle error
}

If the object argument is a pointer, the whole thing can be shortened to an equally (or arguably more) readable:

obj := &demande.Patient{}
if err := dbpkg.Save(db, obj); err != nil {
    // handle error
}
// obj now has the ID set and can be used as the most up-to-date value as stored in the collection

Other comments/observations/recommendations:

Something that kind of bugs me when reading through this is the use of the word Restraints when defining a type constraint. The thing that I dislike most is that it doesn't communicate at all what the constraint actually is. A constraint like Number or comparable gives you an idea of what data is being handled by the function. Are Restraints things that are supposed to filter data sets, like a map reduce kind of thing? Are they validation steps on some data? Are they malicious things that hog 80% of system resources? I would call them something like DTO or MModel (short for MongoModel) or something like that. It at least communicates clearly that we're expecting types that represent data that is stored in some capacity.

A second pet peeve is that you're not passing in the context.Context argument, and instead are using context.Background() hard coded. Just create a context.WithCancel(context.Background()) in your main function, defer the cancel call in said main function (where you'll probably handle signals, start the application, any servers you might be running, etc... and pass the context in. At the very least, by doing that, if your application receives a kill signal while saving a load of data, the context gets cancelled and routines that have access to this context can be made aware of the program exiting by checking <-ctx.Done().

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much for the detailed review and advices !I will refactor my code as soon as I have some time to work on this project. You are right also in the fist time, I was trying to find an excuse to use generics but I guess my approach was bad, I'll keep reading more to identify exactly when it would make sense to use them \$\endgroup\$
    – Russo
    Apr 24, 2022 at 22:21
  • 1
    \$\begingroup\$ @Russo we're all coders here, so I think none of us can blame you for wanting to use a new feature. We all do it. I've been apprehensive WRT generics in go for quite some time, because I know what people, including myself, are like. We might forget the old adage of "right tool for the job". So far, I've actually written just a handful of generic functions, because I was fed up seeing the same old crap being repeated everywhere (stuff like func max(a, b int64) int64 and then the same for uint. Cutting down the crap with func[T Number]Max(a, b T) T is just sensible, not sexy, but right. \$\endgroup\$ Apr 26, 2022 at 15:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.