Construct a URL given an Enum

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?

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.

StackExchange Code Review Q#114722, answer score: 3