3
\$\begingroup\$
private String GetFacilityName(String facilityHexID) {
    if (facilityHexID.equals(FACILITY_AIRCON_HEX_ID)) {
       return FACILITY_AIRCON_NAME;
    }
    else if (facilityHexID.equals(FACILITY_FAN_HEX_ID)) {
       return FACILITY_FAN_NAME;
    }

    return facilityHexID;
}

Or

private String GetFacilityName(String facilityHexID) {
    if (facilityHexID.equals(FACILITY_AIRCON_HEX_ID)) {
       return FACILITY_AIRCON_NAME;
    }
    else if (facilityHexID.equals(FACILITY_FAN_HEX_ID)) {
       return FACILITY_FAN_NAME;
    }
    else {  
       return facilityHexID;
    }
}

They essentially do the same thing. I'm thinking first one is better from the standpoint of "Write more code only if you have to" and the second one is better from readability aspect.

I'd use a switch statement with default case but I'm using a old version of JDK.. so I can't use it on Strings.

Which one is better?

\$\endgroup\$
3
  • 2
    \$\begingroup\$ What about using a Map? \$\endgroup\$
    – tb-
    Feb 21, 2013 at 17:35
  • \$\begingroup\$ How about using a switch? Java7 exists. \$\endgroup\$ Feb 22, 2013 at 10:46
  • \$\begingroup\$ Is it possible to switch over to a custom Enum for your return string? It almost seems like it would best match up with this. I can't find the example on line that showed how to use the toValue version. \$\endgroup\$ Feb 22, 2013 at 18:21

1 Answer 1

10
\$\begingroup\$

I'd remove both elses:

private String getFacilityName(String facilityHexId) {
    if (FACILITY_AIRCON_HEX_ID.equals(facilityHexId)) {
       return FACILITY_AIRCON_NAME;
    }
    if (FACILITY_FAN_HEX_ID.equals(facilityHexId)) {
       return FACILITY_FAN_NAME;
    }
    return facilityHexId;
}

I think it's easier to follow. (It's called Guard Clause.)

Some other notes:

  1. I've switched the equals to CONSTANT.equals(inputParameter). Now it handles null inputs too and does not throw NullPointerException.

  2. I've renamed the method to camelCase. It's more common in Java to start method names with small letters. (Code Conventions for the Java Programming Language, 9 - Naming Conventions)

  3. I've changed ID to Id in the variable names. I prefer camelCase here too because it's usually easier to read. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

    While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ FWIW, Guard Clause is more about checking method invariants prior to logic execution. E.g. if (facilityHexId == null) return null; would be a Guard Clause. \$\endgroup\$ Feb 25, 2013 at 12:38
  • 1
    \$\begingroup\$ @ChrisKnight: I've thought the same when I wrote the answer but "checks for trivial cases, and gets rid of them quickly" (from the linked C2 wiki) seems appropriate here. refactoring.com/catalog/… also reinforces that. What do you think? \$\endgroup\$
    – palacsint
    Feb 25, 2013 at 13:34
  • 1
    \$\begingroup\$ @palacsint: Well, to me, the name 'Guard Clause' implies its guarding against something which isn't the purpose of the if statements in the above code. In the refactoring.com link the line seems blurred between guarding against something and performing execution logic. Interesting link though. \$\endgroup\$ Feb 25, 2013 at 14:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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