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

Encapsulation is broken #104

Closed
laptevn opened this issue Jun 13, 2018 · 7 comments
Closed

Encapsulation is broken #104

laptevn opened this issue Jun 13, 2018 · 7 comments

Comments

@laptevn
Copy link

laptevn commented Jun 13, 2018

To create new Facebook bot using JBot I need to do the following:
1 Define fbSubscribeUrl, fbSendUrl and fbMessengerProfileUrl properties.
2 Define RestTemplate bean.

All these are internal implementation details that JBot users don't care at all. Having them complicates workflow and introduces copy paste from bot example.

I would suggest the following:
1 Hide internal properties behind JBot code and use Bot#getPageAccessToken method where you need page access token.
2 Hide RestTemplate bean behind JBot code.

So after the fix JBot users will need just to add it to dependencies, extend Bot class and use proper annotations.
Easy and quick and no strange magic code is required.

Please share your vision on this. If everybody is happy I will introduce a PR with a fix.

@rampatra
Copy link
Owner

rampatra commented Jun 15, 2018

Hi, thanks for stating your views. I do understand your concern but FbBot is designed this way so that you can have any number of bots in a single application by just extending Bot class and overriding getPageAccessToken.

@laptevn
Copy link
Author

laptevn commented Jun 15, 2018

Really? And how any number of bots relate to broken encapsulation?

@laptevn
Copy link
Author

laptevn commented Jun 15, 2018

Looks like you have no idea what is encapsulation and lazy enough to read and understand my point. Well, at least you had a chance to learn something.

@rampatra
Copy link
Owner

Such hauteur and derisive comments were uncalled for. Was I in anyway rude to you? Anyways, just pontificating won't help anyone here, we both maybe talking two completely different things. If you really want to help you can either raise a PR or at least show some code snippets to expound your point clearly.

Only one point I could deduce from your comments, i.e, instead of creating the RestTemplate bean in JBotApplication class, I can instantiate the bean somewhere inside Jbot so that the user doesn't have to instantiate himself (or copy code). Other points of yours are rather subjective and would be better if you can explain more such as this "Hide internal properties behind JBot code", hide what properties and where??

But do keep in mind that JBot is more of a boilerplate than a library (as per my view) for making bots because of the fact that it is made on spring-boot which is a big dependency. This wasn't the best decision to use spring-boot for JBot but that's another story.

@laptevn
Copy link
Author

laptevn commented Sep 14, 2018

@rampatra
I see you learned what is encapsulation and finally fixed issues I mentioned in this ticket.
How much time you needed to accept this issue? :-D

@laptevn
Copy link
Author

laptevn commented Sep 14, 2018

So you replied with bullshit, then thought for 2 days, found that what I wrote was correct and fixed that here https://github.com/rampatra/jbot/pull/106/commits
And you did it in a hidden way assuming that was your refactoring :-)
I'm curious what is open source for you :-D

@rampatra
Copy link
Owner

@laptevn In the PR you mentioned, I have just moved the application properties to a class as I had many similar url with a slight difference. I actually followed the approach in this PR #95 as it seemed logical to me.

Users still need to do these in their bot classes:

    /**
     * Set this property in {@code application.properties}.
     */
    @Value("${fbBotToken}")
    private String fbToken;

    /**
     * Set this property in {@code application.properties}.
     */
    @Value("${fbPageAccessToken}")
    private String pageAccessToken;

I think you also wanted to get rid of this. Anyways, if my above PR aligns with your suggestion then I am glad. I see you had one more suggestion of taking out RestTemplate. If you can elaborate on it I can work on it later or you're free to open a PR if you wish.

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

No branches or pull requests

2 participants