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

Adding ability to use getters and setters as opposed to public methods. #15

Closed
wants to merge 5 commits into from

Conversation

iainconnor
Copy link

Great work on this library. I was always a fan of the syntax in https://github.com/pardom/ActiveAndroid, but the performance hit against reflection on annotations seemed like a downside so I was really happy to see Ollie pop up.

One of the things I didn't particularly like when integrating with my existing model schema was that all the fields had to be public, so I added a new annotation called @GetSet that tells the generated classes to look for getter and setter methods instead of accessing the fields directly.

@iainconnor
Copy link
Author

Nice idea.

Part of the reason for my original syntax was a thought in the back of my head on whether you should consider making the @Column annotation's parameter optional, and automatically generate it based on the field name, since you know those are unique and meaningful. But thinking a bit more on that, you'd probably want them as known quantities so you can use those final variables for writing queries as well;

new Select().from(Post.class).where(Post.TITLE + "=?", "Foobar");

as opposed to

new Select().from(Post.class).where("title=?", "Foobar");

@pardom-zz
Copy link
Owner

Part of the reason for my original syntax was a thought in the back of my head on whether you should consider making the @column annotation's parameter optional, and automatically generate it based on the field name, since you know those are unique and meaningful.

Yes, that's what I've been wrestling with. However, with your approach we rely on the user keeping the method names up-to-date since methods can't be statically linked to the method name string, which adds another step to refactoring method names.

Another thing I prefer about my syntax is that it enforces the idea that Ollie is processing and referencing those methods. There is no magic happening.

Again, I haven't settled on one particular idea and I'm open to improvements on mine. It doesn't have to be @Column annotations either. It could be @Get(TITLE) and @Set(TITLE) (although we can infer get and set by the return type so something like @Accessor(TITLE) could be better).

@iainconnor
Copy link
Author

Yup, I see what you mean. The way you suggested does keep it nice and clear, and keeps Ollie (and, really, the database) at a nice distance from the models themselves.

Updated to see what it feels like with @Accessor(TITLE) in the mix.

Even though we can infer the get and set, I still might consider making @Accessor and @Mutator separate annotations, just to be consistent with what those terms actually mean.

@pardom-zz
Copy link
Owner

After some thought, I think the correct way to handle this is by convention. If you have a private @Column fooBar field, there should be associated getFooBar and setFooBar fields.

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.

2 participants