Efficiently returning the string basis on current datacenter

Question

We have machine hostname as -

dbx111.dc1.host.com
dbx112.dc2.host.com
dcx113.dc3.host.com
dcx115.dev.host.com

Here dc1, dc2, dc3 and dev are our datacenter and we will be having only four datacenter as of now. And also it might be possible that machine hostname can have more dots in between separated by another domain in future.

Now I need to find out which datacenter my current machine is in as I will be running below code on the actual machine.

And also I have two flows as of now: USERFLOW and DEVICEFLOW.

public enum FlowTypeEnum {
 USERFLOW, DEVICEFLOW
}

Problem Statement:

• If my machine is in DC1 and flow type is USERFLOW then I need to return /test/datacenter/dc1 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc1
• But if my machine is in DC2 and flow type is USERFLOW then I need to return /test/datacenter/dc2 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc2.
• And if my machine is in DC3 and flow type is USERFLOW then I need to return /test/datacenter/dc3 but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc3.
• But if my machine datacenter is in DEV, and flow type is USERFLOW then I need to return "/test/datacenter/dc1" but if flow type is DEVICEFLOW then I need to return /testdevice/datacenter/dc1.

The only difference between USERFLOW and DEVICEFLOW is - For USERFLOW, I need to use /test and for DEVICEFLOW, I need to use /testdevice and other things are same.

TestingEnum class -

public class TestingDatacenter {
 public static void main(String[] args) {
 String LOCAL_POOK = DatacenterEnum.getOurlocation().toLocalPook(FlowTypeEnum.USERFLOW);
 System.out.println(LOCAL_POOK);
 }
}

DatacenterEnum class -

public enum DatacenterEnum {
DEV, DC1, DC2, DC3;
 public static DatacenterEnum getOurlocation() {
 return ourlocation;
 }
 public static String forCode(int code) {
 return (code >= 0 && code < values().length) ? values()[code].name() : null;
 }
 private static final DatacenterEnum ourlocation = compareLocation();
 private static DatacenterEnum compareLocation() {
 String currenthost = getHostNameOfServer();
 if (currenthost != null) {
 if (isDevMachine(currenthost)) {
 return DC1;
 }
 for (DatacenterEnum dc : values()) {
 String namepart = "." + dc.name().toLowerCase() + ".";
 if (currenthost.indexOf(namepart) >= 0) {
 return dc;
 }
 }
 }
 return null;
 }
 public String toLocalPook(FlowTypeEnum f) {
 String prefix = "";
 // below if else, looks pretty odd, may be it can be improved better?
 if (f.equals(FlowTypeEnum.DEVICEFLOW)) {
 prefix = "/testdevice";
 } else if (f.equals(FlowTypeEnum.USERFLOW)) {
 prefix = "/test";
 }
 if (this == DEV) {
 return prefix + "/datacenter/dc1";
 }
 return prefix + "/datacenter/" + name().toLowerCase();
 }
 private static final String getHostNameOfServer() {
 try {
 return InetAddress.getLocalHost().getCanonicalHostName().toLowerCase();
 } catch (UnknownHostException e) {
 // log an exception
 }
 return null;
 }
 private static boolean isDevMachine(String hostName) {
 return hostName.indexOf("." + DEV.name().toLowerCase() + ".") >= 0;
 }
}

I'm opting for code review to see whether there is any better way of doing this. Is there anything which can be simplified and improved?

Answer 1

final is meaningless on static methods :

private static final String getHostNameOfServer() {

For the following :

// below if else, looks pretty odd, may be it can be improved better?
if (f.equals(FlowTypeEnum.DEVICEFLOW)) {
 prefix = "/testdevice";
} else if (f.equals(FlowTypeEnum.USERFLOW)) {
 prefix = "/test";
}

there are 2 alternatives

1. make a getPrefix() method on FlowTypeEnum, reducing the above code to :

   prefix = f.getPrefix();

   FlowTypeEnum could then look like this :

   public enum FlowTypeEnum {
    USERFLOW("/test"), DEVICEFLOW("/testdevice");
    private final String prefix;
    FlowTypeEnum(String prefix) {
    this.prefix = prefix;
    }
    public String getPrefix() {
    return prefix;
    }
   }
   

2. make a map that maps the the FlowTypeEnum instances to a prefix, reducing the above code to :

   prefix = mapToPrefix.get(f);

In both cases you might even inline the prefix variable.

Try to make code resistant to likely changes. In this case, server names and paths are likely to change. Pull these from config files.

All these static methods don't seem to be at home on the DatacenterEnum, they don't really operate on DatacenterEnum instances. Perhaps they need to find a home on a new class, where they can be non static.

It is unwise to hard code the data centers as enum instances. It simply won't scale. Renaming, removing or adding data centers in the field will require a new release.

Use more meaningful names for variables :

• dc -> dataCenter
• f -> flowType

Replace this :

currenthost.indexOf(namepart) >= 0

by the more readable :

currenthost.contains(namepart)

• arsenal: Thanks bowmore for the suggestion. I was not able to understand one thing on FlowTypeEnum - Can you provide an example how f.getPrefix() will work?
• bowmore: I've added sample code for FlowTypeEnum.

Answer 2

Some minors things

You variables names do not always follow conventions.

private static final DatacenterEnum ourlocation = compareLocation();

---

Should be ourLocation. You need to capitalize the first letter of each word in your varaible name.

 } catch (UnknownHostException e) {
 // log an exception
 }

Well you're not logging anything right now, so either you do it or you remove the comment (Nothing to worry, I just hate comments).

---

It's weird to me to have the get method of a variable before the declaration of the variable.

public static DatacenterEnum getOurlocation() {
 return ourlocation;
}

I read a class like a book, so when I encounter something like that I always need to stop my flow, find the variable to see is it a private variable, is someone went crazy and it's from another class. The thing is it's not in order.

---

I don't understand this method :

public static String forCode(int code) {
 return (code >= 0 && code < values().length) ? values()[code].name() : null;
}

What it's suppose to do ? Get the name of the server or the enum ?