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.
CodePudding user response：
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);
}
CodePudding user response：
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)
}
}
CodePudding user response：
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);
}