A good way to refactor code

Question

I have several DTO objects.

public class MovieRequest {}
public class OtherTitle extends MovieRequest {}
public class ReleaseDate extends MovieRequest {}

These objects are uploaded using a generic contribution(DTO object) public class ContributionNew<T extends MovieRequest> {} to the methods

void createOtherTitleContribution(
        final ContributionNew<OtherTitle> contribution,
        ...
);
void updateOtherTitleContribution(
        final ContributionUpdate<OtherTitle> contribution,
        ...
);
 void createReleaseDateContribution(
        final ContributionNew<ReleaseDate> contribution,
        ...
);
void updateReleaseDateContribution(
        final ContributionUpdate<ReleaseDate> contribution,
        ...
);

Information for films is more and these methods will be duplicated about 30 times f. e. createBoxOfficeContribution e.t.c For example, the method of updating contributions looks like this

@Override
public void updateOtherTitleContribution(
        ContributionUpdate<OtherTitle> contribution,
        Long contributionId,
        Long userId
) throws ResourceNotFoundException {
    log.info("Called with contribution {}, contributionId {}, userId {}",
            contribution, contributionId, userId);

    final UserEntity user = this.findUser(userId);
    final ContributionEntity contributionEntity = this.findContribution(contributionId, EditStatus.WAITING, user, MovieField.OTHER_TITLE);

    this.validIds(contributionEntity.getIdsToAdd(), contribution.getElementsToAdd().keySet());
    this.validIds(contributionEntity.getIdsToUpdate().keySet(), contribution.getElementsToUpdate().keySet());
    this.validIds(contributionEntity.getIdsToDelete(), contribution.getIdsToDelete());

    this.cleanUpIdsToAdd(contributionEntity.getIdsToAdd(), contribution.getElementsToAdd().keySet(), contributionEntity.getMovie().getOtherTitles());
    this.cleanUpIdsToUpdate(contributionEntity.getIdsToUpdate(), contribution.getElementsToUpdate().keySet());
    this.cleanUpIdsToDelete(contributionEntity.getIdsToDelete(), contribution.getIdsToDelete());

    contribution.getElementsToAdd().forEach((key, value) -> {
        this.moviePersistenceService.updateOtherTitle(value, key, contributionEntity.getMovie());
    });
    contribution.getElementsToUpdate().forEach((key, value) -> {
        this.moviePersistenceService.updateOtherTitle(value, key, contributionEntity.getMovie());
    });
    contribution.getNewElementsToAdd()
            .forEach(otherTitle -> {
                final Long id = this.moviePersistenceService.createOtherTitle(otherTitle, contributionEntity.getMovie(), user);
                contributionEntity.getIdsToAdd().add(id);
            });

    contributionEntity.setSources(contribution.getSources());
    Optional.ofNullable(contribution.getComment()).ifPresent(contributionEntity::setUserComment);
}

As you can see, the only difference in these methods is to call the method

this.moviePersistenceService.updateOtherTitle(...)

And the method updateReleaseDateContribution calls

this.moviePersistenceService.updateReleaseDate(...)

As you can see the difference is only a few lines, and the old way the current code will have several thousand lines. Do you have any idea for a universal generic method? How to do it better?

Are you using repository to update the entity? – alltej Dec 04 '17 at 17:56 — alltej, Dec 04 '17 at 17:56

score 0 · Answer 1 · answered Dec 04 '17 at 18:58

0

you can create a method moviePersistenceService.update(MovieRequest req) that will check the type of req before doing the persistence part.
You can also pass a Function (the one that will do the persistence part) as parameter to updateOtherTitleContribution (check How to pass a function as a parameter in Java?)

answered Dec 04 '17 at 18:58

Akli REGUIG

552
4
13

1. You mean something that I check e.g if( req instanceof OtherTitle) ? – Dec 04 '17 at 19:52
yes, in your persistence classe because i guess you are storing the data in differente tables (or collections) or something. – Akli REGUIG Dec 04 '17 at 19:56
A little strange look will be over 15 conditions. In Java 8, this is unlikely. – Dec 04 '17 at 20:09
Oh. Ok i thought it was only a few number of entities. So the best solution that i can think of is to create a persistence function for each of your entities and pass that function to updateOtherTitleContribution as suggested in my second point. Maybe someone else can provide with another solution. – Akli REGUIG Dec 04 '17 at 20:15

A good way to refactor code

1 Answers1