Subclass instance recorder in Java

Question

I've written a relatively small abstract class in Java which keeps track of all instances of it's sublasses. Any class which extends this abstract class will automatically have all its instances stored (via a weak reference) in a Set, which can be retrieved if a subclass ever wants to get references to all it's instances.

Although this class is relatively short and simple, I have never made anything like it and so I want to ensure that I'm not doing something terribly wrong or giving in to any bad practices:

import java.util.*;
public abstract class InstanceRecorder {
 private static Map<Class<? extends InstanceRecorder>, Set<InstanceRecorder>> instances;
 static {
 instances = new HashMap<>();
 }
 {
 instances.putIfAbsent(this.getClass(), Collections.newSetFromMap(new WeakHashMap<>()));
 instances.get(this.getClass()).add(this);
 }
 protected static int countInstances(Class<? extends InstanceRecorder> key) {
 if (instances.containsKey(key))
 return instances.get(key).size();
 return 0;
 }
 @SuppressWarnings("unchecked")
 protected static <T extends InstanceRecorder> Set<T> getInstances(Class<T> key) {
 if (instances.containsKey(key))
 return Collections.unmodifiableSet((Set<T>) instances.get(key));
 return new HashSet<>();
 }
}

As I said above, I've never made a class like this before and so any feedback or insight would be greatly appreciated.

• forsvarir: Can you give an example of when you think this might be useful?
• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! Please see What to do when someone answers. I have rolled back Rev 2 → 1
• HomeworkHopper: @Sᴀᴍ Onᴇᴌᴀ, You're correct - It says "Do not add an improved version of the code." My mistake.
• HomeworkHopper: @forsvarir, I was imagining that this would be useful in cases where a programmer would want to call an instance method or change the value of an instance variable on all instances of a certain class. The programmer could, of course, simply use an array for this task, however this class stores instances automatically and also illuminates the risk of a memory leak as it utilizes weak references.

Answer 1

Two very small details:

Instead of using putIfAbsent / get, you can use the single function computeIfAbsent:

instances.computeIfAbsent(
 this.getClass(),
 ign -> Collections.newSetFromMap(new WeakHashMap<>())
).add(this);

Instead of a new HashSet for a not-found-result, you can simply return Collections.emptySet(). As your return value is immutable anyway this is more consistent and does not create an additional object for nothing.

(And just to give another opinion: I prefer not to have braces around single statements - only so that you know there is no unambiguos truth out there :-))

Answer 2

Your code is not thread-safe. You should use ConcurrentHashMap instead of HashMap, if this code is ever run in a multithreaded environment.

In Java 8 there was a terrible performance bug in ConcurrentHashMap.computeIfAbsent, which would lock the whole map even if the key already exists. That bug has been fixed in Java 9. See also this question.

• HomeworkHopper: I completely forgot about Thread safety! Nice catch

Answer 3

in my opinion, the code is good, but I have some suggestions.

1. Instead of using java.util.Map#containsKey, you can directly use java.util.Map#get and check if the value is null.

protected static int countInstances(Class<? extends InstanceRecorder> key) {
 Set<InstanceRecorder> instanceRecorders = instances.get(key);
 if (instances != null) 
 return instanceRecorders.size();
 return 0;
}
@SuppressWarnings("unchecked")
protected static <T extends InstanceRecorder> Set<T> getInstances(Class<T> key) {
 Set<InstanceRecorder> instanceRecorders = instances.get(key);
 if (instanceRecorders != null)
 return Collections.unmodifiableSet((Set<T>) instanceRecorders);
 return new HashSet<>();
}

2. In my opinion, it's a bad practice not to put the braces when you have a single instruction coupled with if, while, for, etc. This can make the code harder to read and can cause confusion.