Construct a URL given an Enum

Question

I have a below code which constructs the URL given a FlowType enum.

private String getURL(FlowType type) throws Exception {
 StringBuilder url = new StringBuilder();
 if (TestUtils.isProduction()) {
 if (type.equals(FlowType.HOLDER)) {
 url.append(VISITOR);
 } else {
 url.append(USER);
 }
 } else {
 if (type.equals(FlowType.HOLDER)) {
 url.append(VISITOR_STAGE);
 } else {
 url.append(USER_STAGE);
 }
 }
 long version = 0;
 if (DataMapping.isInitialized()) {
 if (!TestUtils.isEmpty(DataMapping.getPartition(type))) {
 version = DataMapping.getPartition(type).getVersion();
 }
 }
 url.append("web/hasChanged?ver=" + version);
 return url.toString();
}

I wanted to review this code. Any improvements or suggestions?

Answer 1

Here are a few comments:

• you don't seem to be using any instance fields/methods of the enclosing class. So it seems that you could make the method static
• throwing Exception is not ideal - you should only throw the specific exceptions that the code can throw
• finally the method does several things and you could extract each step in its own method

An initial refactoring could therefore look like:

private static String getURL(FlowType type) throws SpecificException {
 StringBuilder url = new StringBuilder();
 url.append(getUrlRoot(type));
 url.append("web/hasChanged?ver=");
 url.append(getVersion(type));
 return url.toString();
}
private static String getUrlRoot(FlowType type) {
 if (TestUtils.isProduction()) {
 return type == FlowType.HOLDER ? VISITOR : USER;
 } else {
 return type == FlowType.HOLDER ? VISITOR_STAGE : USER_STAGE;
 }
}
private static long getVersion(FlowType type) {
 if (DataMapping.isInitialized()) {
 Partition partition = DataMapping.getPartition(type);
 if (!TestUtils.isEmpty(partition)) return partition.getVersion();
 }
 return 0;
}

That leaves one code smell which is that you seem to access a lot of "static state" via the TestUtils and DataMapping classes. Without knowing what they do it's hard to tell if this is good or bad (likely: bad).

The getUrl method should probably not need to know that these static methods exist so I would try to remove the calls and pass the required information as an argument of the getUrl method.