2
\$\begingroup\$

I have this Enum from which I am calling the appropriate execute method based on what type of enum (eventType) is passed.

public enum EventType {

  EventA {
    @Override
    public Map<String, Map<String, String>> execute(Map<String, String> eventMapHolder) {
      if (eventMapHolder.isEmpty()) {
        return ImmutableMap.of();
      }
      String clientId = eventMapHolder.get("clientId");
      Map<String, String> clientInfoHolder = getClientInfo(clientId);
      eventMapHolder.putAll(clientInfoHolder);
      return ImmutableMap.<String, Map<String, String>>builder().put(EventA.name(), eventMapHolder)
          .build();
    }
  },
  EventB {
    @Override
    public Map<String, Map<String, String>> execute(Map<String, String> eventMapHolder) {
      if (eventMapHolder.isEmpty()) {
        return ImmutableMap.of();
      }
      return ImmutableMap.<String, Map<String, String>>builder().put(EventB.name(), eventMapHolder)
          .build();
    }
  },
  EventC {
    @Override
    public Map<String, Map<String, String>> execute(Map<String, String> eventMapHolder) {
      if (eventMapHolder.isEmpty()) {
        return ImmutableMap.of();
      }
      String clientId = eventMapHolder.get("clientId");
      Map<String, String> clientInfoHolder = getClientInfo(clientId);
      eventMapHolder.putAll(clientInfoHolder);
      return ImmutableMap.<String, Map<String, String>>builder().put(EventC.name(), eventMapHolder)
          .build();
    }
  },
  EventD {
    @Override
    public Map<String, Map<String, String>> execute(Map<String, String> eventMapHolder) {
      if (eventMapHolder.isEmpty()) {
        return ImmutableMap.of();
      }
      Map<String, Map<String, String>> holder = new HashMap<>();
      String clientId = eventMapHolder.get("clientId");
      Map<String, String> clientInfoHolder = getClientInfo(clientId);
      eventMapHolder.putAll(clientInfoHolder);
      holder.put(EventD.name(), eventMapHolder);

      Map<String, String> processInfoHolder = getProcessInfo(clientId);
      holder.put(EventE.name(), processInfoHolder);
      return Collections.unmodifiableMap(holder);
    }
  },
  EventE {
    @Override
    public Map<String, Map<String, String>> execute(Map<String, String> eventMapHolder) {
      return ImmutableMap.of();
    }
  };

  public abstract Map<String, Map<String, String>> execute(String eventMapHolder);

  public Map<String, String> getClientInfo(final String clientId) {
    // code to populate the map and return it
  }

  public Map<String, String> getProcessInfo(final String clientId) {
    // code to populate the map and return it
  }   
}

For example: If I get "EventA", then I am calling its execute method. Similarly, if I get "EventB" then I am callings its execute method, and so on.

String eventType = String.valueOf(payload.get("eventType"));
Map<String, String> eventMapHolder = payload.get("eventMapHolder");
Map<String, Map<String, String>> processedMap = EventType.valueOf(eventType).execute(eventMapHolder);

In general, I will have more event types (around 10-12) in the same enum class and mostly they will do the same operation as EventA, EventB, EventC and EventD. In my above enum, I have EventE just to be used as a constant name. Its execute method won't be called at all since I have an abstract execute method, so I have to override it. As you can see, in my EventD, I am populating maps using EventD and EventE as the key.

Question:

Now as you can see, code in the execute method of EventA and EventC are identical but the only difference is what I put as "key" (event name) in the returned immutable map. Similarly, there might be another event as well which will have the same exact code as EventD. Is there any way to remove that duplicated code but still achieve the same functionality in the enum?

The idea is, based on what type of event is passed, I want to call its execute method, perform some operation and try to avoid duplication if possible. Also, I am thinking of moving getClientInfo and getProcessInfo in some other utility class so that this Enum class should have execution code only.

If there is any other better way or any other design pattern to do this then I am open for suggestions as well which can help me remove duplicated code.

\$\endgroup\$
4
  • \$\begingroup\$ I've noticed you're asking quite a bunch of java / kafka questions recently. Do note that advice on questions asked in short succession can be repetitive, especially if you don't wait for a review. Additionally note, that there's a badge where asking three questions (or is it 4?) in one day is equivalent to asking one question in one day :) \$\endgroup\$ – Vogel612 Dec 18 '16 at 14:12
  • \$\begingroup\$ Yeah I am almost finished coding on my recent project so I thought to get some feedback. Didn't knew those kind of badges exist :) \$\endgroup\$ – david Dec 18 '16 at 18:35
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Dec 18 '16 at 19:49
  • \$\begingroup\$ Sure. That was my mistake. I will keep that in mind next time for sure. Thanks for keeping me in check. \$\endgroup\$ – david Dec 18 '16 at 19:53
2
\$\begingroup\$

In general, I will have more event types (around 10-12) in the same enum class and mostly they will do the same operation as EventA, EventB, EventC and EventD.

Then you should simply call execute on the other enum constant that already has this "mostly same operation".

In my above enum, I have EventE just to be used as a constant name. Its execute method won't be called at all since I have an abstract execute method.

This reasoning is wrong. The abstract method declaration you have is a guarantee that all constants have this method implemented.

If you don't want to implement the method execute in each and every enum constant you should change the abtract method declaration into a "normal" method (with (empty) body and without the abstract keyword), like you have for the other methods.

Now as you can see, code in the execute method of EventA and EventC are identical but the only difference is what I put as "key" (event name) in the returned immutable map.

Change Event?.name() to this.name() in both constants and then use your IDEs "extract method" feature to move the code out of the constants into a private enum method to be called from both enums.


[edit]

There is no such feature in Eclipse

You should learn to know your tool better:

In eclipse:

  • Select the code you want to extract (e.g. doubleclick behind the openning curly brace of the constants method)
  • right click on the the selection
  • in the context menu point to "refactoring" -> "Extract Method"

in the dialog that opens

  • fill in a (good) method name in "Method Name"
  • from the "Destination" check box select your enums name.

if you did the suggested change on all enums the last line now tells how many occurrences of the same code will be replaced with a call to the new method.

\$\endgroup\$
9
  • \$\begingroup\$ Looks like you are using Netbeans and I use Eclipse. There is no such feature in Eclipse unfortunately as per my understanding. :( If you can provide an example what does my extract method will look like, then that will make my understanding clear. I know what you meant but mostly confused how do I put event name in the key then? \$\endgroup\$ – david Dec 18 '16 at 18:39
  • \$\begingroup\$ @david Eclipse most certainly has extract method. Select the part you want to extract and press ctrl alt m \$\endgroup\$ – Vogel612 Dec 18 '16 at 19:05
  • \$\begingroup\$ Oops never used that feature. Just saw it. Working on it. \$\endgroup\$ – david Dec 18 '16 at 19:17
  • \$\begingroup\$ @TimothyTruckle So both for EventA and EventC execute method, I changed key to this.name() in the returned immutable map. Now I have extracted the common code (basically which is everything) for EventA and EventC into a separate method by using the extract feature. And then I see compilation error in extract method in the map where I put key which is obvious I know but wanted to confirm, is that what you meant? But the confusion I have is how to put key in the immutable map then? \$\endgroup\$ – david Dec 18 '16 at 19:33
  • \$\begingroup\$ @TimothyTruckle or you mean my extract method should also take EventType enum name as a String parameter? And then use that in the immutable map? \$\endgroup\$ – david Dec 18 '16 at 19:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.