3
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.