NewService function has too many arguments

[lunemec]

func NewImportService(jsonRepo repository.Import, orgRepo repository.Organization, locRepo repository.Location, contactRepo repository.Contact) *ImportService {
	return &ImportService{
		// Code omitted.
	}
}

This is terrible.
Suppose having 50 DB tables, it is hidious and unreadable. But we can't use (repositories ...tableInterface) because we could not be certain all required repositories were provided.
Another issue here is when adding new repositories, the compiler message gets unreadable when missing one of 20 repositories.

[thfre]

Supply it with an array of objects which implement an importable interface which has an import function. You can use reflection to find all your objects in repositories which implement it. You need to do the same thing on all of them, so they are not really distinct dependencies. Or have I missed something? :)

[lunemec]

@Phr0ztByte that would work, but if any of the "required" repositories were missing, I'd have to check that in runtime. This way it wont compile. That is what I would like.

[thfre]

@lunemec Add them to the array manually. It would still save your constructor and if it does not implement importable it would throw a compile time error

[lunemec]

But how would you distinguish between the individual repositories within the service? Since you'd expect []Importable how do you test for it having repository.Organization interface? This seems worse than having too many arguments.

[thfre]

Why would you need to know? I might not have understood the purpose of the service. The import function on the repo would import the data itself.

[lunemec]

Well, the service uses the repositories to Get/Save data in the DB. If you'd pass them as Importable how would you than save data from the service? Could you post an example of what you mean? Maybe that will clarify our discusion :)

EDIT:

// Import uses jsonRepository to load data and save them in DB repositories.
func (i *ImportService) Import(id uint) error {
	imported, err := i.jsonRepo.Get(id)
	if err != nil {
		return err
	}

	saved, err := i.orgRepo.Save(imported)
	if err != nil {
		return err
	}

	// Here would be more code handling recursive saving of locations, contacts, etc...
	// Handling cases where you want to update existing records, delete stale etc...

	fmt.Printf("Saved: %v\n", saved)
	return nil
}

[lunemec]

@Phr0ztByte for some reason I got email with your comment, but can't see it here ...

[thfre]

Ah. Your original solution is better than trying to tidy up the constructor. There are other solutions, but it depends on the context of which ImportService is used. It really is a special case, since it touches almost your entire DB. Hard to encapsulate responsibility.

[sanggonlee]

I realize this is an old thread but how about having a separate ImportServiceOptions struct?

type ImportServiceOptions struct {
  JSONRepository repository.Import,
  ...
}

And either passing it to the constructor

func NewImportService(o *ImportServiceOptions) (ImportService, error)

or initialize from the options object itself.

Although you still cannot validate "required" repositories at compile time, it's easier to write validator function that checks them at the earliest runtime possible.

If you do need 50 or similar dependencies I don't see any other way around if not introducing another layer of abstraction.

[lunemec]

@sanggonlee yes I thought about this approach, some libraries use this pattern. But I dislike it because you can pass empty Options struct and it will compile just fine. But when you run it you get runtime errors. But with the other (ugly) approach you can't compile this.

Anyway after some discussion, we came to the conclusion that having a separate interface Repository for each table is not a good idea with Go. In few projects we used https://github.com/volatiletech/sqlboiler and it worked great for us.

The layer separation should be for the transport + logic, but if you use DB - that kinda is part of the logic (in SQL statements).

[sanggonlee]

Thanks for sharing what you came up with. Yeah if you do need the check at compile time that might be the only way to go about it...