Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Code below is scattered all over the code base :

if (StringUtils.contains(manager.getName, "custom")){

}

It just checks if an object attribute contains a predefined String and if it does, enter condition.

Is there a more elegant way of achieving above, perhaps using an enum?

share|improve this question
1  
Is the string being searched for always the same? – Konrad Rudolph Jul 19 '12 at 12:43
@Konrad Rudolph yes its always the same – user470184 Jul 19 '12 at 13:37

3 Answers

up vote 3 down vote accepted
  1. It seems data envy. I'd create nameContains method in the Manager class.

    public boolean nameContains(final String searchText) {
        if (StringUtils.contains(name, searchText)) {
            return true;
        }
        return false;
    }
    
  2. Instance variables (fields) should be private. See: why instance variables in java are always private

share|improve this answer
6  
if (cond) return true; else return false; => return cond; – Konrad Rudolph Jul 19 '12 at 12:43
+1 for the link to data envy. However, as Konrad suggested, the body of this method should be a one-liner. – Leonid Jul 22 '12 at 18:27
+1 for the data envy link – Brian Agnew Aug 1 '12 at 8:08

You can use a static reference for "custom" :

    // In References.class declared 'final'
public final class References{
    static String refSearched = "custom";

    public static boolean methodControl1(final String s){
        return s.contains(refSearched);
    }    
    .... // other fields/methods
}

    // In other classes
    if(References.methodControl1(newStringToSearchIn)){
        ..;
    }
share|improve this answer

Are the strings like "custom" pre-defined? If the number of such strings are limited, then you would also profit by making them into methods such as Manager.hasCustom() if not, you can go for Manager.has(Manager.CUSTOM) where CUSTOM could be an attribute in the Manager class.

Consider memoizing the lookup.

class Manager {
  public final String CUSTOM = "custom";
  public boolean has(String key) {
    if (!map.containsKey(key)) map.put(StringUtils.contains(name, key))
    return map.get(key);
  }
  public boolean hasCustom() { return has(CUSTOM); }
}

Finally, if your class takes decisions based on a flag in another class, you might want to rewrite it to reduce coupling. And perhaps use a pattern like Strategy.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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