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

Some improvements #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .classpath
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
<attribute name="maven.pomderived" value="true"/>
</attributes>
</classpathentry>
<classpathentry kind="src" output="target/test-classes" path="src/test/java">
<attributes>
<attribute name="optional" value="true"/>
<attribute name="maven.pomderived" value="true"/>
</attributes>
</classpathentry>
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.7">
<attributes>
<attribute name="maven.pomderived" value="true"/>
Expand Down
18 changes: 13 additions & 5 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,24 @@
</build>

<dependencies>

<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace looks odd, there's a tabs vs spaces issue here.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, will be fixed.

<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.11</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.glassfish</groupId>
<artifactId>javax.json</artifactId>
<version>1.0-b02</version>
</dependency>
<!-- <dependency>
<groupId>javax</groupId>
<artifactId>javaee-api</artifactId>
<version>7.0-b72</version>

<!-- <dependency>
<groupId>javax</groupId>
<artifactId>javaee-api</artifactId>
<version>7.0-b72</version>
</dependency> -->
Copy link
Member

Choose a reason for hiding this comment

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

Could/should remove commented out dependency?

Copy link
Author

Choose a reason for hiding this comment

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

Please take a look at original one.

Copy link
Member

Choose a reason for hiding this comment

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

Done, will be in my PR!

</dependencies>
</project>
38 changes: 19 additions & 19 deletions src/main/java/JSR353/JSON/PricesCacheStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,37 @@

import java.util.concurrent.ConcurrentHashMap;

public class PricesCacheStore {
public enum PricesCacheStore {

INSTANCE;

private final static PricesCacheStore INSTANCE = new PricesCacheStore();

private PricesCacheStore() {}
private volatile String lastSymboleUpdated;

// will contain Symbols and prices stored and constantly updated
private volatile ConcurrentHashMap<String, Double> pricesCache = new ConcurrentHashMap<>();

public static PricesCacheStore getInstance() {
return INSTANCE;
}


// will contain Symbols and prices stored and constantly updated
Copy link
Member

Choose a reason for hiding this comment

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

Symbole should be Symbol without the 'e'

Copy link
Author

Choose a reason for hiding this comment

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

Please take a look at original. I don't added this 'e'

Copy link
Member

Choose a reason for hiding this comment

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

In my PR, fixed!

private volatile ConcurrentHashMap<String, Double> pricesCache = new ConcurrentHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

volatile CHM? Are you sure you want to do this? Doesn't make sense to have this volatile.

Copy link
Author

Choose a reason for hiding this comment

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

Please take a look at original. I don't added this volatile : ]

Copy link
Member

Choose a reason for hiding this comment

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

This one was from me, I could submit a change request through helio! On rethinking Volatile seems an overkill here.

Copy link
Member

Choose a reason for hiding this comment

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

I'd submit your own Pull Request for that - it's good for us to start learning to use Git/GitHub as it was intended :-)

Copy link
Member

Choose a reason for hiding this comment

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

My PR with the fix is on the way!


public void addPrice(String symbol, Double price) {
pricesCache.put(symbol, price);
lastSymboleUpdated = symbol;
if (symbol != null && price != null) {
pricesCache.put(symbol, price);
lastSymboleUpdated = symbol;
}
}

public Double getPrice(String symbol) {
if (symbol == null) {
return 0d;
}
Double price = pricesCache.get(symbol);
return price == null ? 0 : price;
return price == null ? 0d : price;
}

// Keep in mind immutability - return a copy of the object
// does not matter if it is static or volatile
// or if its threading is safe
public ConcurrentHashMap<String, Double> getAllPrices() {
public ConcurrentHashMap<String, Double> getAllPrices() {
return new ConcurrentHashMap<String, Double>(pricesCache);
}
Copy link
Member

Choose a reason for hiding this comment

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

Not convinced that this is cloning properly.

Copy link
Author

Choose a reason for hiding this comment

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

Me neither. But i don't changed it, please take a look at original : ]

Copy link
Member

Choose a reason for hiding this comment

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

From me again, what is an alternative way to clone, I thought the new constructor is suppose to take in the object and return a new copy after replicating the data.

Copy link
Member

Choose a reason for hiding this comment

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

So that kind of makes sense, but why clone a data structure that is inherently threadsafe?

Copy link
Member

Choose a reason for hiding this comment

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

Thats me, you should know me by now! Doubly safe - overkill again! But it was an oversight as well, as previous to this being a ConcurrentHashMap, it was just HashMap - if i remember (before I even created the repo). So that lingered along into this commit.

Will return the object instead since thats clear - new PR coming!


public String getLastSymbolUpdated() {
return new String(lastSymboleUpdated);
}
Expand Down
Loading