Notifying players about certain events

This class looks ugly as hell to me. I feel like there is a better way but can't really think for a good one. Too many instanceofs and else ifs. What OOP practices or design patterns would you suggest to change and improve the code?

Any tips, advises and recommended resources are welcome!

What does the code do: Basically, public method accepts event object and every event object contains user/player object in some form. The reason we are getting user object from the event is to check whether that particular user will receive notification about the event. Which of course depends on user's notification settings which is inside user object.

I've added ENUM class, maybe there is a way to modify it for a better code quality.

```
@Service
public class NotificationSettingsCheckServiceImpl implements NotificationSettingsCheckService {

@Override
public Boolean checkIfEventCanPass(ApplicationEvent event) {

if (SYSTEM_EVENTS.getListOfClasses().contains(event.getClass())) {
return true;
}

Boolean eventPasses = false;
LotteryUser user;
user = event instanceof NotificationApplicationEvent ? getUserBasedOnNotificationEvent(event) : getUserBasedOnEmailEvent(event);

if (CAMPAIGN_EVENTS.getListOfClasses().contains(event.getClass()) && user.getNotificationSettings().getCampaignEvents()) {
eventPasses = true;
} else if (DRAW_RESULT_EVENTS.getListOfClasses().contains(event.getClass()) && user.getNotificationSettings().getDrawResultEvents()) {
eventPasses = true;
} else if (TRANSACTION_EVENTS.getListOfClasses().contains(event.getClass()) && user.getNotificationSettings().getTransactionEvents()) {
eventPasses = true;
} else if (USER_WON_EVENTS.getListOfClasses().contains(event.getClass()) && user.getNotificationSettings().getTransactionEvents()) {
eventPasses = true;
}

return eventPasses;
}

private LotteryUser getUserBasedOnEmailEvent(EventObject event) {

LotteryUser user = null;

if

Boolean vs boolean

Prefer to use the primitive type boolean instead of Boolean (The difference is, simply put, that Boolean can also be null).

Varargs, defensive copy

You're passing a list of classes to your enum constructor. You could instead use varargs and pass Class... (or possibly Class... although that might give you a warning, if so just ignore it). Then you can get rid of wrapping in Arrays.asList when calling the constructor and instead do it in the constructor itself.

Also, don't return the list as it is, return a copy of it. Otherwise some calling code can do getListOfClasses().clear(); and screw up everything. In fact, all you really need is the contains method of the list, so skip the getListOfClasses method and make a contains method on your enum instead.

Also, make listOfClasses final.

Improved code:

private final List> listOfClasses;

NotificationSettingsType(Class... classes) {
    this.listOfClasses = Arrays.asList(classes);
}

public boolean contains(Class clazz) {
    return listOfClasses.contains(clazz);
}

Getting a user...

Let the EventObject itself know how to get a user. Make a method somewhere, depending on your class heirarchy for your events, that returns a LotteryUser based on email or based on notification event. Then you can simply call this:

private LotteryUser getUserBasedOnEmailEvent(EventObject event) {
    return event.getUserByEmail();
}

private LotteryUser getUserBasedOnNotificationEvent(EventObject event) {
    return event.getUserByNotification();
}

This removes the need for these methods completely. Depending on whether or not each EventObject really has multiple users, you could even do return event.getUser();

StackExchange Code Review Q#162709, answer score: 4