3
\$\begingroup\$

I have written the following method which works as expected. But is there a way to write this more elegantly in Java 8? Appreciate any feedback. Thanks.

private String get(Details details, Value value){
    Type type = Type.getEvent(details.getType());

    if(type != null){
        if("SAMPLE".equals(type.typeDesc())){
            if(SPIN.equals(details.getSpin())){ // this if looks very similar to last line.. 
                return BACKUP + SAMPLE + value.getId();
            }
            return DEFAULT_VAL + SAMPLE + value.getId();
        }
        return value.getRoute();
    }
    return StringUtils.contains(details.getSpin(), SPIN) ? BACKUP : DEFAULT_VAL;
}
\$\endgroup\$

closed as off-topic by Roland Illig, Linny, dfhwze, Toby Speight, pacmaninbw Oct 10 at 12:31

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Roland Illig, Linny, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ Could you edit the title to state what the code does? \$\endgroup\$ – dfhwze Oct 9 at 16:13
  • 2
    \$\begingroup\$ And then please edit the introduction text as well. \$\endgroup\$ – Roland Illig Oct 10 at 3:43
4
\$\begingroup\$

I think it's a personal thing, but I prefer == null checks with early exits, because that way you have all exception paths at the beginning and the normal execution path is less indented. For your string concatenation you could make a ternary if and have one return statement less.

private String get(Details details, Value value){
    Type type = Type.getEvent(details.getType());
    if(type == null) {
      return StringUtils.contains(details.getSpin(), SPIN) ? BACKUP : DEFAULT_VAL;
    }

    if("SAMPLE".equals(type.typeDesc())) {
        String prefix = SPIN.equals(details.getSpin()) ? BACKUP : DEFAULT_VAL;
        return prefix + SAMPLE + value.getId();
    }
    return value.getRoute();
}

Now looking at this, you check the spin for equals and contains. Is this intentional? If not you could further reduce the code by pulling the prefix part up like this:

private String get(Details details, Value value){
    String prefix = SPIN.equals(details.getSpin()) ? BACKUP : DEFAULT_VAL;

    Type type = Type.getEvent(details.getType());
    if(type == null) {
      return prefix;
    }

    if("SAMPLE".equals(type.typeDesc())) {

        return prefix + SAMPLE + value.getId();
    }
    return value.getRoute();
}

In general you should use StringBuilder for string concatenation.

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

Giving a similar answer to the one already provided.

I'm a big fan of "return a value for an edge case" pattern. If there's a very specific case with its own return value, then it's more readable (to me..) to eliminate the special case in the beginning of the function, and then to focus on the regular flow.

In your case:

private String get(Details details, Value value){
    Type type = Type.getEvent(details.getType());
    // apply the "early return" pattern
    if(type == null){
        return StringUtils.contains(details.getSpin(), SPIN) ? BACKUP : DEFAULT_VAL;
    }

    // apply the same pattern again
    if(!"SAMPLE".equals(type.typeDesc())){
        return value.getRoute();
    }

    // finally, the "regular" flow
    String prefix = SPIN.equals(details.getSpin()) ? BACKUP : DEFAULT_VAL
    return prefix + SAMPLE + value.getId();
}
\$\endgroup\$
1
\$\begingroup\$

I'm mostly concerned with this line:

if("SAMPLE".equals(type.typeDesc())){

Comparison to a type string is a strong clue that there's an anti-pattern in this code. If the types are constrained to certain known values, then at the very least this should be tracked with an enumeration.

More likely, though, is that you should do the traditional OOP thing: instead of writing polymorphism-by-if-statement, write polymorphism-by-class-derivation. In other words, this get method would be implemented in a base class, and then overridden in child classes where appropriate. No type check would take place except for the implicit type check done in the JVM when a base class method is invoked. Even if there is only one parent class and one Sample child class, that would be a better-structured and more maintainable solution than what I suspect is happening in your code right now.

All of that said: it's difficult to know exactly what to do, what's feasible or even preferable without seeing more of your code. If you show more of the Type code I can expand on this.

\$\endgroup\$
  • 1
    \$\begingroup\$ Without knowing any context of this code, it's brave to say that splitting the code into several classes makes it more maintainable. \$\endgroup\$ – Roland Illig Oct 10 at 3:47

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