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

Standardize builder/factory/provider language #552

Closed
mdekstrand opened this issue Apr 1, 2014 · 6 comments
Closed

Standardize builder/factory/provider language #552

mdekstrand opened this issue Apr 1, 2014 · 6 comments
Labels
Milestone

Comments

@mdekstrand
Copy link
Member

@mdekstrand mdekstrand commented Apr 1, 2014

Right now, we use FooBuilder both for a class exposing an API to build a complex Foo, and for building model objects.

The latter, since they mostly just get their get method called and are configured via the constructor, should possibly be called FooFactory.

In general, we should be consistent and comprehensible in our usage of the terms builder, factory, and provider.

@mdekstrand mdekstrand added this to the 3.0 milestone Apr 1, 2014
@mdekstrand mdekstrand added the proposal label Apr 1, 2014
@mdekstrand mdekstrand changed the title Rename component builders to factories Standardize builder/factory/provider language May 20, 2015
@mdekstrand

This comment has been minimized.

Copy link
Member Author

@mdekstrand mdekstrand commented Oct 9, 2015

Proposal:

  • Introduce ComponentFactory interface and @DefaultFactory annotation, and make them equivalent to a Provider and @DefaultProvider, except higher-priority.
  • Call model builders model factories. It's weird to call this expensive building process a 'provider', in my opinion.
  • Use javax.inject.Providers for lightweight dynamic provision, rather than expensive model building.
  • Reserve the ‘builder’ terminology for Effective Java 2-style object builders, where we're just setting up the contents of an object to be built and are operating outside of the DI context. This is more consistent with general use of 'builder', as well as some other LensKit use of the term.

The downside I see to this strategy is the overlap between ‘factory’ and ‘provider’. Maybe it's OK to have an ItemItemModelProvider that does an expensive build process.

@kluver, thoughts?

@kluver

This comment has been minimized.

Copy link
Contributor

@kluver kluver commented Oct 11, 2015

What actual value does this work give us? Ive never had confusion around these points as factories and provides seem to fill the same role of being an object that causes an object.

In general I want to avoid duplication, especially meta-level duplication.

@mdekstrand

This comment has been minimized.

Copy link
Member Author

@mdekstrand mdekstrand commented Oct 13, 2015

Avoiding duplication is good, and explaining the difference between a provider and a factory is probably not worthwhile, especially since there is no actual difference in how they are used.

So, standardize on 'Provider' language and rename all our 'model builders' to be 'model providers', and go with that?

@kluver

This comment has been minimized.

Copy link
Contributor

@kluver kluver commented Oct 13, 2015

That works for me.

@mdekstrand

This comment has been minimized.

Copy link
Member Author

@mdekstrand mdekstrand commented Oct 13, 2015

Will do.

mdekstrand added a commit to mdekstrand/lenskit that referenced this issue Jul 8, 2016
mdekstrand added a commit to mdekstrand/lenskit that referenced this issue Jul 8, 2016
mdekstrand added a commit to mdekstrand/lenskit that referenced this issue Jul 8, 2016
mdekstrand added a commit to mdekstrand/lenskit that referenced this issue Jul 8, 2016
@mdekstrand

This comment has been minimized.

Copy link
Member Author

@mdekstrand mdekstrand commented Jul 17, 2016

I have renamed all of our builders that should be providers in #941. Done.

@mdekstrand mdekstrand closed this Jul 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.