-2
\$\begingroup\$

Is it possible to merge these two cases into one so I don't have what is basically duplicated code except for the cast part?

       case ScheduleActionsMediator.ACTION_CREATE_RENDITION:
            final List<CrewMemberVO> crewMembers = new ArrayList<CrewMemberVO>();
            CrewMemberRenditionActionStruct struct = (CrewMemberRenditionActionStruct) action;
            for (final CrewMemberVO crew : struct.getAffectedCrews().values())
                if (crew != null)
                    crewMembers.add(crew);
            this.cacheFacade.deleteCrewMembers(crewMembers, false, this.crewMemberRules);
            break;
        case ScheduleActionsMediator.ACTION_CREATE:
            final List<CrewMemberVO> crewMembersTwo = new ArrayList<CrewMemberVO>();
            CrewMemberCreateModifyActionStruct structTwo = (CrewMemberCreateModifyActionStruct) action;
            for (final CrewMemberVO crew : structTwo.getAffectedCrews().values())
                if (crew != null)
                    crewMembersTwo.add(crew);
            this.cacheFacade.deleteCrewMembers(crewMembersTwo, false, this.crewMemberRules);
            break;
\$\endgroup\$
2
  • 3
    \$\begingroup\$ More context, please! What is this code supposed to do? What is the switch statement? Are these the only two cases? \$\endgroup\$ Jun 26 '17 at 21:02
  • 1
    \$\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\$
    – Mast
    Jun 27 '17 at 14:06
2
\$\begingroup\$
    case ScheduleActionsMediator.ACTION_CREATE_RENDITION:
    case ScheduleActionsMediator.ACTION_CREATE:
        List<CrewMemberVO> crewMembers = new ArrayList<>();
        for (final CrewMemberVO crew : action.getAffectedCrews().values()) {
            if (crew != null) {
                crewMembers.add(crew);
            }
        }
        cacheFacade.deleteCrewMembers(crewMembers, false, crewMemberRules);
        break;

If action's type supports the getAffectedCrews method, this will work as stands. This gets rid of the duplicate code, which will make it easier to maintain.

Of course, we don't have sufficient context here to see if action supports the getAffectedCrews method without casting. If it doesn't, you'd have to make changes outside of this snippet in order to make things work. It's unclear how extensive those changes might be. If you post a followup question, consider posting at least the entire method. We'd need the type for action and how it relates to the two cast types. Also include the ScheduleActionsMediator definition.

I removed the this. from several variables. Unless there is a conflict, you don't need it. Of course, some people prefer to use it to indicate which are object fields rather than local variables.

I removed the final from crewMembers, as not telling us much. It doesn't keep a mutable type like ArrayList from being modified. It only keeps which object is represented from being changed.

The final on crew is more useful, but not by much.

In newer versions of Java, you don't need to specify <CrewMemberVO> the second time. You can just say <>. Not only does that save you some typing now, but it helps simplify future maintenance.

\$\endgroup\$
2
  • \$\begingroup\$ All the 'final' and 'this' are configured in Eclipse's save actions as per our project manager preferences and what you did was my first try, but action.getAffectedCrews() requires the cast. I edited my answer to show what I ended up doing. \$\endgroup\$ Jun 27 '17 at 13:59
  • \$\begingroup\$ I mean, I answered my own question with what I did, if you could provide feedback I would be thankful. \$\endgroup\$ Jun 27 '17 at 14:10
0
\$\begingroup\$

I guess you can make the common part as a function, then call it with different parameter from different branches.

\$\endgroup\$
0
\$\begingroup\$

After talking to some more people we've reached the conclusion that we could create an interface and have CrewMemberRenditionActionStruct and CrewMemberCreateModifyActionStruct implement it and the method getAffectedCrews, but that would be overkill.

Here's what I ended up doing:

        case ScheduleActionsMediator.ACTION_CREATE_RENDITION:
            this.deleteCrewMembersFromCache(((CrewMemberRenditionActionStruct) action).getAffectedCrews().values());
            break;
        case ScheduleActionsMediator.ACTION_CREATE:
            this.deleteCrewMembersFromCache(((CrewMemberCreateModifyActionStruct) action).getAffectedCrews().values());
            break;

And here's the private method:

  private void deleteCrewMembersFromCache(final Collection<CrewMemberVO> values) throws WplexError
    {
    final List<CrewMemberVO> crewMembers = new ArrayList<CrewMemberVO>();
    for (final CrewMemberVO crew : values)
        if (crew != null)
            crewMembers.add(crew);
    this.cacheFacade.deleteCrewMembers(crewMembers, false, this.crewMemberRules);
    }
\$\endgroup\$

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.