current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

I am writing a utility method which can check for empty and null string, or collection or an object or any general types -

public static boolean isEmpty(Object obj) {
    if (obj == null)
        return true;
    if (obj instanceof Collection)
        return ((Collection<?>) obj).size() == 0;

    // is below line expensive?
    final String s = String.valueOf(obj).trim();

    return s.length() == 0 || s.equalsIgnoreCase("null");
}

How can I make my above method efficient, since above isEmpty method will be called multiple times from the application which is very performance critical?

I am suspecting below line will be expensive because of heavy toString methods and it will create temporary garbage as well that might cause GC and slow down the performance?

final String s = String.valueOf(obj).trim();

If I need to check for map null or empty, should I keep both collection isEmpty and Map isEmpty method both or Collection isEmpty method will be fine for that?

public static void main(String[] args) {

    Map<String, String> hello = new HashMap<String, String>();
    System.out.println(isEmpty(hello));

    Map<String, HashMap<Integer, String>> primary = new HashMap<String, HashMap<Integer, String>>();
    System.out.println(isEmpty(primary));

}

public static boolean isEmpty(Collection<?> value) {
    return value == null || value.isEmpty();
}

public static boolean isEmpty(Map<?, ?> value) {
    return value == null || value.isEmpty();
}
share|improve this question
    
Why would you need to do this? This shouldn't be necessary under most circumstances. Additional information about the use case could help us give you a more appropriate solution. –  AJMansfield Jul 6 '14 at 17:07

2 Answers 2

up vote 10 down vote accepted

Method overloading can make your implementations more efficient and cleaner:

public static boolean isEmpty(Collection obj) {
    return obj == null || obj.isEmpty();
}

public static boolean isEmpty(String string) {
    return string == null || string.trim().isEmpty();
}

public static boolean isEmpty(Object obj) {
    return obj == null || obj.toString().trim().isEmpty();
}

The Collection version is as efficient as possible.

The String version would be more efficient without the trimming. It would be best to trim your strings as soon you see them, long before they reach this call. If you can review the callers and make sure that the strings are always trimmed at their origins, then you can remove .trim() for best performance.

The Object version can be inefficient, depending on the toString implementation of the objects that will be passed to it, and because of the trimming.

I removed the comparison with null from there, because it seems pointless to me. I mean, a class whose toString method says "null" would seem very very odd.

In any case, you don't really want the Object version to be called, at all. Most importantly because it probably won't even work. Take for example an empty Map. Its toString method returns the string {}, which won't match your conditions of emptiness. (For this type you should definitely add isEmpty(Map<?, ?> map) to benefit from its isEmpty method.)

If performance is so critical, then add more overloaded implementations for all other types that you care about, for example:

public static boolean isEmpty(Something obj) {
    return obj == null || obj.isEmpty();
}

Finally, especially when something is so important, you definitely want to unit test it, for example:

@Test
public void testEmptyObject() {
    assertTrue(isEmpty((Object) null));
    assertFalse(isEmpty(new Object()));
}

@Test
public void testEmptyString() {
    assertFalse(isEmpty("hello"));
    assertTrue(isEmpty(""));
    assertTrue(isEmpty(" "));
    assertTrue(isEmpty((Object) null));
}

@Test
public void testEmptySet() {
    assertFalse(isEmpty(new HashSet<String>(Arrays.asList("hello"))));
    assertTrue(isEmpty(new HashSet<String>()));
}

@Test
public void testEmptyMap() {
    Map<String, String> map = new HashMap<String, String>();
    assertTrue(isEmpty(map));
    map.put("hello", "hi");
    assertFalse(isEmpty(map));
}
share|improve this answer
    
Thanks for suggestion. If I need to use for Map, then do I need to add separately for Maps? Or collection one will be fine? –  lining Jul 5 '14 at 23:05
    
@Webby A Map is not a Collection (which can be seen from the javadoc), so you need to add one separately for Map. –  Simon André Forsberg Jul 5 '14 at 23:07
    
@janos Thanks. Makes sense now. I have updated the question with an example of Maps. I am using two maps example, one is simple String and String and other is Maps within Map. So I can use same isEmpty method for both map example right? –  lining Jul 5 '14 at 23:14
    
@Webby You need to keep both because again, a Map is not a Collection. And a SimonAndréForsberg is not a janos. –  Simon André Forsberg Jul 5 '14 at 23:34
up vote 8 down vote

Don't.

I mean. Don't use the same method for all kinds of objects.

This method does not make much sense to me.

This line smells. A lot.

if (obj instanceof Collection)
    return ((Collection<?>) obj).size() == 0;

Beware of instanceof operator.

I am sure that whatever it is that you are trying to do here, there are better ways to do it.

Java is a statically typed language, use the static types whenever possible. If you really don't know what type the object have, then I will provide another alternative below.

// is below line expensive?
final String s = String.valueOf(obj).trim();

That depends, on the implementation of the object's toString method.

The implementation of String.valueOf is:

public static String valueOf(Object obj) {
    return (obj == null) ? "null" : obj.toString();
}

return s.length() == 0 || s.equalsIgnoreCase("null");

You have already checked for obj == null. The string will only be null when the object's toString method makes it so. And instead of s.length() == 0 you can use s.isEmpty() directly. (Although that is implemented as string length == 0

Do it differently

If possible, have the types of objects you're investigating implement an interface that provides an isEmpty method and let the object decide for itself if it is empty or not.

If that is not possible, you can use a dynamically created map with ways to determine whether or not the object is "empty".

Map<Class<?>, EmptyChecker> map = new HashMap<>();
map.put(String.class, new StringEmptyChecker());
map.put(Point.class, new PointEmptyChecker());

This is a kind of Strategy pattern.

Then to determine if an object is empty:

EmptyChecker checker = map.get(obj.getClass());
checker.isEmpty(obj);

The whole thing is kinda weird though, I can't really see a particular use-case for this kind of method.

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.