Switch statement results in duplicated code

Question

I'm a junior dev, and looking to better organize my code.

Right now, I'm dealing with form numbers. I retrieve a hash map of form numbers, and based on the form number, I need to call a different method. Each method takes in the same parameters, but does something different.

For example:


var formDetails = new inferForms.buildFormsMap

for(form in formDetails){

switch(form.formNumber){
          case "A1345":
            getExclusionDetails(account, state, form, businessDealing)
            break
          case "B254":
            getExclusionDetails(account, state, form, businessDealing)
            break
          case "B297":
            getPartnershipDetails(account, state, form, businessDealing)
            break
          case "C397":
            getBrokerageDetails(account, state, form, businessDealing)
            break
          case "D972":
            getBrokerageDetails(account, state, form, businessDealing)
            break
          case "E192":
            getBrokerageDetails(account, state, form, businessDealing)
            break
          case "E299":
            getBrokerageDetails(account, state, form, businessDealing)
            break
          case "F254":
            getLocationDetails(account, state, form, businessDealing)
            break
          case "F795":
            getLocationDetails(account, state, form, businessDealing)
            break
          case "G642":
            getContractDetails(period, wcmJurisdiction, newForm, wcmBusiness, frm)
            break
          case "G979":
            getContractDetails(period, wcmJurisdiction, newForm, wcmBusiness, frm)
            break
   }
}

A few notes:

-The methods were built by another dev. He quit, so I am assuming his work and looking to refactor to make this better.

-The starting point is a HashMap of form numbers. I generate the HashMap, and then loop through it to gather details based on each form number in the HashMap.

-Even if I were to convert the methods into an object inheritance structure, I would still need a switch statement to know which subclass to instantiate, no? And the switch statement would look like the one above?

-Some of these case statements are calling the exact same method. Is there a way to avoid duplication?

Thanks for all your help. I'm pulling my hair out trying to figure out how to better re-engineer this. Please let me know if I can provide additional details.

You can use [switch statement fall through](https://stackoverflow.com/questions/188461/switch-statement-fall-through-should-it-be-allowed) — Michał Krzywański, Jun 09 '22 at 15:37

MrSmith42 · Answer 1 · 2022-06-10T16:39:11.647

At least some cases have the same body -> use switch statement fall through

switch(form.formNumber){
      case "A1345": // fall through
      case "B254":
        getExclusionDetails(account, state, form, businessDealing)
        break;
      case "B297":
        getPartnershipDetails(account, state, form, businessDealing)
        break
      case "C397": // fall through
      case "D972": // fall through
      case "E192": // fall through
      case "E299":
        getBrokerageDetails(account, state, form, businessDealing)
        break
      case "F254": // fall through
      case "F795":
        getLocationDetails(account, state, form, businessDealing)
        break;
      case "G642": // fall through
      case "G979":
        getContractDetails(period, wcmJurisdiction, newForm, wcmBusiness, frm)
        break;
}

See comment by Holger:

With JDK 14 and newer, you can use the new syntax allowing multiple labels, without fall-through.

switch(form.formNumber) { 
  case "A1345", "B254" -> getExclusionDetails(account, state, form, businessDealing); 
  case "B297" -> getPartnershipDetails(account, state, form, businessDealing); 
  case "C397", "D972", "E192", "E299" -> getBrokerageDetails(account, state, form, businessDealing); 
  case "F254", "F795" -> getLocationDetails(account, state, form, businessDealing); 
  case "G642", "G979" -> getContractDetails(period, wcmJurisdiction, newForm, wcmBusiness, frm); 
}

score 0 · Answer 2 · answered Jun 09 '22 at 15:52

You could replace the switch case with if, else if, since there are multiple conditions with the same outcome, it will reduce the repetitions.

        var formDetails = new inferForms.buildFormsMap

        for(form in formDetails){
            var formNumber = form.formNumber
            if(formNumber.equals("A1345") || formNumber.equals("A1345")){
                getExclusionDetails(account, state, form, businessDealing)
            } else if (formNumber.equals("B297") || formNumber.equals("C397")) {
                getPartnershipDetails(account, state, form, businessDealing)
            } else if (formNumber.equals("D972") || formNumber.equals("E192")) {
                getBrokerageDetails(account, state, form, businessDealing)
            } else if (formNumber.equals("F254") || formNumber.equals("F795")) {
                getLocationDetails(account, state, form, businessDealing)
            } else if (formNumber.equals("G642") || formNumber.equals("G979")) {
                getContractDetails(period, wcmJurisdiction, newForm, wcmBusiness, frm)
            }
        }

score 0 · Answer 3 · answered Jun 09 '22 at 20:22

Your case seems like a good candidate for factory pattern to me.

Start with defining an abstraction for gathering different details.

public interface DetailsManager {

  void gatherDetails(String account, String state, String form, String businessDealing);
}

Continue with concrete implementations.

public class ExclusionDetailsManager implements DetailsManager {

  @Override
  public void gatherDetails(String account, String state, String form, String businessDealing) {
    //do stuff
  }
}

public class PartnershipDetailsManager implements DetailsManager {

  @Override
  public void gatherDetails(String account, String state, String form, String businessDealing) {
    //do other stuff
  }
}

To avoid switch and if-else statements you can use Map in the factory.

public class DetailsManagerFactory {

  private final Map<String, Supplier<DetailsManager>> map;
  private final Supplier<DetailsManager> defaultSupplier;

  public DetailsManagerFactory(Map<String, Supplier<DetailsManager>> map) {
    this.map = map;
    this.defaultSupplier = DefaultDetailsManager::new;
  }

  public DetailsManager getManager(String formNumber) {
    return this.map.getOrDefault(formNumber, this.defaultSupplier).get();
  }

  private static final class DefaultDetailsManager implements DetailsManager {

    @Override
    public void gatherDetails(String account, String state, String form, String businessDealing) {
      //default manager doing nothing, just making sure not to cause NPE
    }
  }
}

Creation of the DetailsManager is delayed by being wrapped in a Supplier. This can be useful if the object is heavyweight - instances are created only when needed. If that's not needed, you can just change the values of the map to DetailsManager.

public class CachingDetailsManagerSupplier implements Supplier<DetailsManager> {

  private final Supplier<DetailsManager> managerSupplier;
  private DetailsManager cache;

  public CachingDetailsManagerSupplier(Supplier<DetailsManager> managerSupplier) {
    this.managerSupplier = managerSupplier;
  }

  @Override
  public DetailsManager get() {
    if (this.cache == null) {
      //init manager
      this.cache = this.managerSupplier.get();
    }
    return this.cache;
  }
}

This supplier keeps created instance cached, but depending on your exact use case this might udesirable/unneeded.

Example

//init factory where appropriate
Supplier<DetailsManager> exclusionManagerSupplier = new CachingDetailsManagerSupplier(ExclusionDetailsManager::new);
Map<String, Supplier<DetailsManager>> map = new HashMap<>();
map.put("A1345", exclusionManagerSupplier);
map.put("B254", exclusionManagerSupplier);
map.put("B297", new CachingDetailsManagerSupplier(PartnershipDetailsManager::new));
DetailsManagerFactory factory = new DetailsManagerFactory(map);

//gather details
for (Object form : formDetails) {
  String formNumber = form.formNumber;
  DetailsManager manager = factory.getManager(formNumber);
  manager.gatherDetails(account, state, form, businessDealing);
}

Switch statement results in duplicated code

3 Answers3