Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a method to get datastore map #520

Merged
merged 7 commits into from
Oct 22, 2020
Merged

Conversation

haroon-sheikh
Copy link
Contributor

No description provided.

}
return null;
}

public static synchronized ConcurrentHashMap<Object, Object> getMap() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DataStore deliberately avoided returning a Map to hide the internal storage implementation. Exposing this would make changing the implementation of the DataStore harder in the future.

Is there a good reason why the getMap methods needs to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just looking to get a list of entries. I can change these to return an entrySet instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now updated to return entrySet only which we also did previously in the old DataStore.

return map.get();
}

public static synchronized Set<Map.Entry<Object, Object>> entrySet() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah Ok.

From https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#entrySet--

public Set<Map.Entry<K,V>> entrySet()
Returns a Set view of the mappings contained in this map. 
The set is backed by the map, so changes to the map are reflected in the set, 
and vice-versa. If the map is modified while an iteration over the set is in progress
 (except through the iterator's own remove operation, or through the setValue 
operation on a map entry returned by the iterator) the results of the iteration are 
undefined. The set supports element removal, which removes the corresponding
 mapping from the map, via the Iterator.remove, Set.remove, removeAll, retainAll 
and clear operations. It does not support the add or addAll operations.

The new DataStore was introduced to prevent modification of the map.
To get over this it's better to return an immutable Map
https://docs.oracle.com/javase/9/core/creating-immutable-lists-sets-and-maps.htm#JSCOR-GUID-4F3E2B7D-CE90-4862-A78A-414FC08DA6E4

You can call that method items

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now updated to return a immutable Map.

Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@zabil
Copy link
Member

zabil commented Oct 22, 2020

Thanks for making the changes @haroon-sheikh thought a bit more about this and the problem with returning the full map will create two ways of accessing the store values i.e.

ScenarioDataStore.get('key');
ScenarioDataStore.items().get('key');

Which may be redundant.
As the idea is to iterate or go through the values in the store can be try something like this instead?

         public static synchronized Set<Object> items() {
            return Collections.unmodifiableSet(map.get().keySet());
         }

The here is a re-implementation of the DataStore to show the usage

package com.company;

import java.util.Collections;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

public class Main {
     static class ScenarioDataStore {
        private static ThreadLocal<ConcurrentHashMap<Object, Object>> map = ThreadLocal.withInitial(ConcurrentHashMap::new);

        public static synchronized void put(Object key, Object value) {
            if (key != null && value != null)  {
                map.get().put(key, value);
            }
        }

        public static synchronized Object remove(Object key) {
            if (key != null) {
                return map.get().remove(key);
            }
            return null;
        }

        public static synchronized Object get(Object key) {
            if (key != null) {
                return map.get().get(key);
            }
            return null;
        }

         public static synchronized Set<Object> items() {
            return Collections.unmodifiableSet(map.get().keySet());
         }

        static synchronized void clear() {
            map.get().clear();
        }
     }

    public static void main(String[] args) {
       ScenarioDataStore.put("1", "one");
       ScenarioDataStore.put("2", "two");
       ScenarioDataStore.put("3", "three");
       ScenarioDataStore.items().forEach((k) -> System.out.println(ScenarioDataStore.get(k)));
    }
}

This way there is only one way to access or add items to the store. Hope this makes sense.

Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@haroon-sheikh
Copy link
Contributor Author

Thanks for the feedback @zabil. I've now made the suggested changes.

@zabil zabil added feature ReleaseCandidate If a PR is tagged with this label, after merging it will be released labels Oct 22, 2020
@zabil
Copy link
Member

zabil commented Oct 22, 2020

Marked this for release. Can you bump up the version? You can refer dce766b on how to do that.

Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ReleaseCandidate If a PR is tagged with this label, after merging it will be released
Development

Successfully merging this pull request may close these issues.

2 participants