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

Class explosion #661

Open
urbandroid opened this issue Mar 29, 2023 · 9 comments
Open

Class explosion #661

urbandroid opened this issue Mar 29, 2023 · 9 comments

Comments

@urbandroid
Copy link

Right now there is no way to avoid class explosion problem like decorator pattern of gang of four with CDI.

I suggest it should be added like this:

@FacebookDecorator(@SlackDecorator(@SmsDecorator)) 
Notification notification;

for this example

@Ladicek
Copy link
Contributor

Ladicek commented Mar 30, 2023

This issue is extremely vague. It might be just me, but I can't understand what the objection here really is.

For the record, it is impossible to declare annotations in Java so that you can compose them like this: @FacebookDecorator(@SlackDecorator(@SmsDecorator)).

@manovotn
Copy link
Contributor

This issue is extremely vague. It might be just me, but I can't understand what the objection here really is.

Same here, more details would be nice :)

BTW, CDI decorator isn't meant to work exactly like the decorator design pattern, if that's the issue. Instead, it is closer to interceptors in its functionality but gives you more power as you have more information about the class you are decorating at the time of invocation.

@urbandroid
Copy link
Author

Google "class explosion" problem once you know that it is pretty clear what the problem is actually.

@manovotn
Copy link
Contributor

That doesn't explain much in terms of how is this related to CDI specification?
CDI isn't a design pattern cookbook 🤷

@urbandroid
Copy link
Author

"CDI isn't a design pattern cookbook" i never said that it was. if you didn't understand the problem pass it to someone who does.

to explain to problem to who didn't get it, lets say we have Notifier class and injected in SmsNotifier, FacebookNotifier and SlackNotifier and lets say we want to combine notifiers right now CDI doesn't have a solution to Class explosion so we have to create SmsAndFacebookNotifier, SmsAndSlackNotifier, FacebookAndSlackNotifier, SmsFacebookAndSlackNotifier classes. 2^n -1 classes needed for each base Notifier class, for 3 base class 7 combinator class, for 5 class 31 combinator class it explodes exponentially hence the name Class explosion. CDI doesn't provide a way to solve this issue and it is ugly and cumbersome.

That is why there needs to be way to combine injected classes with CDI

@FacebookDecorator(@SlackDecorator(@SmsDecorator)) Notification notification;

if this is not possible then maybe combining with decorator would do like this:

@Decorated @FacebookDecorator @SlackDecorator @SmsDecorator Notification notification;

@Ladicek
Copy link
Contributor

Ladicek commented Apr 13, 2023

Please stop the passive-aggressive tone.

CDI decorators can actually do what you want with zero class explosion, looking roughly like this:

interface Notifier {
    void notify(Message m);
}

@ApplicationScoped
class DummyNotifier implements Notifier {
    void notify(Message m) {
        // intentionally empty, exists only as an ultimate delegate for decorators
    }
}

@Decorator
@Priority(1)
class FacebookNotifier implements Notifier {
    @Inject 
    @Delegate
    Notifier delegate;

    void notify(Message m) {
        sendFacebookNotification(m);
        delegate.notify(m);
    }
}

@Decorator
@Priority(2)
class SlackNotifier implements Notifier {
    @Inject 
    @Delegate
    Notifier delegate;

    void notify(Message m) {
        sendSlackNotification(m);
        delegate.notify(m);
    }
}

@Decorator
@Priority(3)
class SmsNotifier implements Notifier {
    @Inject 
    @Delegate
    Notifier delegate;

    void notify(Message m) {
        sendSmsNotification(m);
        delegate.notify(m);
    }
}
@ApplicationScoped
class MyService {
    @Inject
    Notifier notifier;

    void doSomething() {
        notifier.notify(new Message("done something"));
    }
}

What might be problematic, depending on the use case, is the declaration-site approach to composition. All injection points of type Notifier get the same "sequence" of decorators.

I assume that what you want is a use-site composition approach, so that each injection point can specify which decorators it wants applied. Do I understand it correctly?

If so, I think something like this might be possible:

@Inject
@DecoratedWith({SmsNotifier.class, SlackNotifier.class})
Notifier noFacebookHere;

Though I didn't really think this through, there might be problems with that that I don't realize right now.

If you have any other ideas, feel free to share, but please, with a complete explanation of the expected behavior. So far, your examples leave a lot for the reader to invent themselves.

@dmatej
Copy link

dmatej commented Apr 13, 2023

Hmm. With such amount of notifiers I would probably rather implement a notifier service with some finer logic. I would expect that sooner or later it would complicate even more.

@Ladicek
Copy link
Contributor

Ladicek commented Apr 13, 2023

Yeah, I generally agree the best solution for composing notifications to external systems is a dedicated service (not necessarily a dedicated program, but that might be an option too). CDI only gets you so far.

@urbandroid
Copy link
Author

"I assume that what you want is a use-site composition approach, so that each injection point can specify which decorators it wants applied. Do I understand it correctly?"

Yes that is correct.

"Hmm. With such amount of notifiers I would probably rather implement a notifier service with some finer logic."
it is just a example there might be better examples to address the issue which won't be solved with service.

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

4 participants