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

Feature/trblock #637

Draft
wants to merge 5 commits into
base: development
Choose a base branch
from
Draft

Conversation

Cupelt
Copy link
Member

@Cupelt Cupelt commented Dec 11, 2023

No description provided.

@github-actions github-actions bot added package:bukkit package:core Related to share domain nor script interpreter labels Dec 11, 2023
Copy link
Member

@wysohn wysohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't wait to see this go live :)

import java.util.logging.Logger;

@Singleton
public class TRBlockManager {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good

But I doubt if having TRBlockManager and TRBlock would make sense since they seem to be doing closely related jobs

If the concern is executing public boolean start() only once, we could put that into initialize() directly instead.

If you prefer having multiple layers of abstraction (which is good), consider creating TRBlock as an interface and creating the TRBlockImpl class which implements the TRBlock interface. In this way, we can avoid depending directly on Javalin. Or it doesn't have to be TRBlock but some more generic name like TRBlockWebServiceProvider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i got it

@Cupelt
Copy link
Member Author

Cupelt commented Dec 13, 2023

@wysohn how about like this?

@wysohn
Copy link
Member

wysohn commented Dec 14, 2023

@wysohn how about like this?

Pretty close, but not quite

Here is how I envisioned:

Before)

image

After)

image

So the point is that in 'before' scenario, TRBlockManager, which is part of TriggerReactor, depends on TRBlock, and TRBlock depends on Javalin, and this makes our project strongly depending on the changes of Javalin

Instead, we let TRBlockManager to depend on TRBlockWebService interface, and make an implementation (JavalinWebService), which will depend on Javalin, so TRBlockManager will no longer depend on Javalin, but it's depending on the abstraction (TRBlockWebService)

@Cupelt
Copy link
Member Author

Cupelt commented Dec 18, 2023

@wysohn how about like this?

Pretty close, but not quite

Here is how I envisioned:

Before)

image

After)

image

So the point is that in 'before' scenario, TRBlockManager, which is part of TriggerReactor, depends on TRBlock, and TRBlock depends on Javalin, and this makes our project strongly depending on the changes of Javalin

Instead, we let TRBlockManager to depend on TRBlockWebService interface, and make an implementation (JavalinWebService), which will depend on Javalin, so TRBlockManager will no longer depend on Javalin, but it's depending on the abstraction (TRBlockWebService)

I understood

@Cupelt
Copy link
Member Author

Cupelt commented Dec 18, 2023

@wysohnI'm not sure if I understood well, but is this what you meant?

@wysohn
Copy link
Member

wysohn commented Dec 18, 2023

@wysohnI'm not sure if I understood well, but is this what you meant?

Yes exactly!

As a minor point, the interface name can be TRBlockWebService instead of TRBlockWebServiceImpl cause it's an abstraction, not an implementation

If you prefer to add postfix, Ipml, then JavalinWebService could be JavalinWebServiceImpl, but I think JavalinWebService that implements TRBlockWebService sounds good enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:bukkit package:core Related to share domain nor script interpreter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants