Notifying players about certain events

Question

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 (event instanceof UserThanksEvent) {
        user = ((UserThanksEvent) event).getUser();
    } else if (event instanceof UserOrderCanceledEvent) {
        user = ((UserOrderCanceledEvent) event).getUser();
    } else if (event instanceof DrawResultEvent) {
        user = ((DrawResultEvent) event).getPlayer();
    } else if (event instanceof UserWinCongratulationEvent) {
        user = ((UserWinCongratulationEvent) event).getPlayer();
    } else if (event instanceof UserAddedToCampaignEvent) {
        user = ((UserAddedToCampaignEvent) event).getLotteryUser();
    }

    return user;
}


private LotteryUser getUserBasedOnNotificationEvent(EventObject event) {

    LotteryUser user = null;

    if (event instanceof UserReceivedBonusMoneyEvent) {
        user = ((UserReceivedBonusMoneyEvent) event).getLotteryUser();
    } else if (event instanceof UserReceivedBonusInNonDepositCampaignEvent) {
        user = ((UserReceivedBonusInNonDepositCampaignEvent) event).getLotteryUser();
    } else if (event instanceof UserReceivedBonusInDepositCampaignEvent) {
        user = ((UserReceivedBonusInDepositCampaignEvent) event).getLotteryUser();
    } else if (event instanceof UserTakesPartInDepositCampaignEvent) {
        user = ((UserTakesPartInDepositCampaignEvent) event).getUser();
    } else if (event instanceof UserTakesPartInNonDepositCampaignEvent) {
        user = ((UserTakesPartInNonDepositCampaignEvent) event).getUser();
    } else if (event instanceof DrawResultNotificationEvent) {
        user = ((DrawResultNotificationEvent) event).getPlayer();
    }

    return user;
 }
}

Here is the ENUM class I'm using.

public enum NotificationSettingsType {

SYSTEM_EVENTS(Arrays.asList(OnRegistrationCompleteEvent.class,
        ResetPasswordEvent.class,
        ManagerRegistrationEvent.class,
        DrawResultNotFoundEvent.class,
        CurrencyUpdatedEvent.class,
        WalletLockedEvent.class,
        UserWonBigPrizeEvent.class)),

CAMPAIGN_EVENTS(Arrays.asList(UserReceivedBonusInDepositCampaignEvent.class,
        UserReceivedBonusInNonDepositCampaignEvent.class,
        UserReceivedBonusMoneyEvent.class,
        UserTakesPartInDepositCampaignEvent.class,
        UserTakesPartInNonDepositCampaignEvent.class,
        UserAddedToCampaignEvent.class)),

DRAW_RESULT_EVENTS(Arrays.asList(DrawResultNotificationEvent.class,
        DrawResultEvent.class)),

TRANSACTION_EVENTS(Arrays.asList(UserThanksEvent.class,
        UserOrderCanceledEvent.class)),

USER_WON_EVENTS(Collections.singletonList(UserWinCongratulationEvent.class));

private List<Class> listOfClasses;

NotificationSettingsType(List<Class> listOfClasses) {
    this.listOfClasses = listOfClasses;
}

public List<Class> getListOfClasses() {
    return listOfClasses;
   }

 }

Here are the Event Object example and the class that it extends. I have several of these depending on the event they notify. This is just an example. Using Lombok plugin for Getter/Setter annotations if that confuses anyone.

 @Getter
 @Setter
 public class UserReceivedBonusMoneyEvent extends NotificationApplicationEvent {

private final LotteryUser lotteryUser;
private final BigDecimal bonusAmount;
private final String currency;

public UserReceivedBonusMoneyEvent( LotteryUser lotteryUser, BigDecimal bonusAmount, String currency) {
    super(bonusAmount);
    this.lotteryUser = lotteryUser;
    this.bonusAmount = bonusAmount;
    this.currency = currency;
}

@Override
public void accept(NotificationEventVisitor visitor) {
    visitor.visit(this);
  }
 }

And here is the NotificationApplicationEvent, which extends Spring framework's Application Event.

 public abstract class NotificationApplicationEvent extends ApplicationEvent {
/**
 * Create a new ApplicationEvent.
 *
 * @param source the object on which the event initially occurred (never {@code null})
 */
public NotificationApplicationEvent(Object source) {
    super(source);
}

public abstract void accept(NotificationEventVisitor visitor);
}

And This is NotificationEventListener class where checkIfEventCanPass method is placed.

@Component
@Slf4j
public class NotificationEventListener implements ApplicationListener<NotificationApplicationEvent> {
@Autowired
private NotificationEventVisitor notificationEventVisitor;

@Autowired
private NotificationSettingsCheckService notificationSettingsCheckService;

@Override
public void onApplicationEvent(NotificationApplicationEvent event) {

    if (notificationSettingsCheckService.checkIfEventCanPass(event)) {
        event.accept(notificationEventVisitor);
    }
 }
}

Answer 1

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<Class<?>> 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();