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

Prototype #90

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Prototype #90

wants to merge 4 commits into from

Conversation

rbattenfeld
Copy link

Factory based API proposal

@ALRubinger
Copy link
Member

Overall I think the method naming and capitalization conventions here need to be reviewed. And I'm not sure the prototype offers a complete view of all the types we'll encounter. Also I'd like to see some JavaDocs/explanation of some of the method names where noted.

The factory approach taken here, however (and I'm changing my mind on this position), is clearly preferable to the hierarchical object model approach we'd taken with "up()", so that's good. Last I spoke to @aslakknutsen on it, that was his opinion as well.

@rbattenfeld
Copy link
Author

Thanks for your comments. I also see this the preferable way. Do you want that I cleanup the prototype?

You are absolutely right, there are naming and capitalization convention not applied I was just hacking in something that shows the concept and is executable.

Shall I wait until you, Aslak and others are able to make their or more comments suggestions?

For me, one important thing is the Factory class. I prefer a class like this. It makes it very simple to create a new objects, especially within IDEs. If you all agree in such a class, then I see it as a static class but how such a static class can be made visible to the users? This class must be generated. Perhaps there is a second factory class that defines all the convenient classes.

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

Successfully merging this pull request may close these issues.

None yet

3 participants