Return to Answer

Your code is thread-safe as it is. [But not really... see the comments below.]

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class. Create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.

Your code is thread-safe as it is. [But not really... see the comments below.]

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class. Create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.

Your code is thread-safe as it is.

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class. Create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.

Your code is thread-safe as it is. [But not really... see the comments below.]

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class. Create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.

Your code is thread-safe as it is.

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class.

• You actually don't need ClientData at all: you can just instantiate a ConcurrentHashMap in the main thread and pass it along to the secondary thread when you start it. It's possible that you have a lot more code than you showed and that you can't really get rid of ClientData. In that case create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.

Your code is thread-safe as it is.

• Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains information related to host names.

• Try to have each method doing only one thing. For example, callServiceURL should just return some json. Then parseResponse should take that json and return a list of host names. Finally, some other functions should add the host names to the set of blocked hosts.

• volatile has the same effect as AtomicReference and makes the code simpler. See this post.

• Instead of either AtomicReference or volatile you could just have a final ConcurrentHashMap which you would clear when you receive a new list.

• Your non-class ClientData with only static methods makes me cringe. Please make it a normal class. Create an instance of ClientData in the main thread and pass it to the secondary thread when you create it.