40
You have several different options for this:
Guava
Google's Guava Library introduces the idea of a Multiset which is capable of counting the occurrences, and also provides a couple of other features.
Java 8
If you are using Java 8 (which I highly recommend if you have the ability to do so), your tokenFound method can simply be this:
occurrences.merge(...
39
Your code is very neat and clean, there is only one thing that I would change:
private static Map<String, String> mMap;
to
private static final Map<String, String> mMap = Collections.unmodifiableMap(initializeMapping());
The reasons:
By not declaring it final and not using a call to unmodifiableMap it is mutable, and it would be possible to ...
23
You could change:
if (mMap.containsKey(code)) {
return mMap.get(code);
}
return null;
to:
return mMap.get(code);
since HashMap.get() returns null if there is no such key in the map.
15
This may be just a matter of preference, but it would seem more useful to me to have the map store the instance of Maps rather than the description directly, because then you also get for free the ability to find other information (the enum entry name, its ordinal value, etc). You may not need this, but as it is very simple to do, I would definitely ...
15
I would say yes, use an array.
Arrays afford very good memory locality, which is good for cache performance. They essentially are the most basic and efficient hash map if the key is the index.
You only need one array with 256 elements: first count up the number of times each character appears in the first string, and then count down for the number of ...
15
Headers
#include <map>
#include <set>
#include <list>
#include <cmath>
#include <ctime>
#include <deque>
#include <queue>
#include <stack>
#include <string>
#include <bitset>
#include <cstdio>
#include <limits>
#include <vector>
#include <climits>
#include <cstring>
#...
14
You made a look-up table. Here's 2 things to note:
Your look up table is between 2 types, but your table is only 1 type. You break type safety. They are enums and it's 'safe' if the enums have the same underlying type, but dirty either way.
A switch frequently generates a look up table for you, and in this case definitely will on any decent compiler. It ...
14
No, this is not the correct way to implement a hash map in Java, and especially not to implement the java.util.Map interface.
You aren't even explicitly implementing the Map interface.
What is (Map[]) Array.newInstance(Map.class, capacity) supposed to be? I'd think a simple new Map[capacity] would be sufficient.
You calculate your hashes yourself. However, ...
14
Comparing two maps is linear, so that's not a big problem.
I don't think you've made particularly good use of maps though. As I see it, you have two choices. You could use an unordered_multiset, or you could use a unordered_map<char, size_t>. In the latter case, you'd keep a count of each character in the string, and increment the count for each ...
11
Besides what has already been mentioned:
public class HashMapImpl {
class HashMap {
What's going on here? HashMapImpl is never used. This should be just simply
public class HashMap {
Don't have your unit tests inside of the class implementation, unit tests should be in a separate folder structure that mirrors your source structure. That way you don't ...
11
Using a try-with-resources
You're correctly closing your Scanner at the end of the method in a finally block, so there can be no resource leaks.
However, starting with Java 7, you can simply use the try-with-resources construct to make this easier:
try (Scanner sc = new Scanner(text)) {
// ...
}
Reading words with lines
You're using a Scanner to ...
10
Guava's Multiset and its AtomicLongMap are designed for this kind of counting.
See also:
Guava's new collection types, explained.
Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)
9
The code is correct. However, I still have some recommendations:
Sort the includes, so you can easily spot recurring/missed ones
#include <algorithm>
#include <iostream>
#include <map>
#include <utility>
#include <vector>
You do not need to run the loop, you can simply pass the map to the std::vector constructor. As you use ...
8
There are a couple of inefficient things you do, and there's some better-practice items too.
First, let's take one of the methods:
public static List<String> loadNodes2_00() {
List<String> node = new ArrayList<>();
try {
InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes2.00.properties");...
8
For this class, lazy initialization brings too little benefit relative to the complication it adds. You only have one member in the enum, and each member of the enum is trivially simple to construct. Therefore, you should populate the map at class-loading time using a static initializer block. (As a bonus, it's guaranteed that the initialization happens ...
8
It looks about perfect.
You got the post increment wrong though:
MapValueIterator<K, V> operator++(int) {
return (*this)++;
}
I believe this goes into an infinite recursion.
Thus this is normally implemented as:
MapValueIterator<K, V> operator++(int) {
MapValueIterator<K, V> result(*this); // get a copy for return
...
8
I have one other addition to what Loki has already pointed out.
The C++ committee has voted to deprecate inheriting from std::iterator as of C++17. The current recommendation is apparently to write those typedefs yourself.
template<typename K, typename V>
class MapValueIterator {
public:
typedef V value_type;
typedef std::ptrdiff_t ...
8
Every instance in an enum already has an ordinal (the 0-based position of the value in the declaration order of the enum). For example, your instance C.ordinal() will return 2. See: Enum.ordinal(). These are the same values as the ones you are assigning to test. Is that a coincidence?
Additionally, you're using a small range of 0-based values for the test ...
8
Initialization of store as a list of None does not really buy you anything in terms of efficiency, but complicates the code quite a bit. I recommend to initialize it with empty lists instead.
put iterates the list twice. A single pass is sufficient:
for i in list_at_index:
if i == p:
i.value = p.value
return
...
8
Quite frankly it seems that you did not do what was asked from you.
The text above assign gives you the requirements
// Assign value val to interval [keyBegin, keyEnd).
// Overwrite previous values in this interval.
// Conforming to the C++ Standard Library conventions, the interval
// includes keyBegin, but excludes keyEnd.
// If !( ...
7
Your code is pretty good as-is. There are other ways to do the same thing, but yours is fine.
One thing I noticed is that you've skipped braces on the if...else in reverseFromMap. My advice is to always use braces, even for one-line expressions.
However, you could also do
reversedMap[value] || (reversedMap[value] = []);
reversedMap[value].push(key);
You ...
7
The HashEntry[] table is currently public in HashMap, which is dangerous,
as anybody can mess with it. It should be private.
The same goes for the fields of HashEntry.
Something's wrong here in get:
while (runner.next != null) {
if (runner.key == key) {
return runner.value;
}
}
runner.key will never be equal to key. And runner.next is ...
7
This is pretty simple code, with not a lot of room for improvement.
You can make the session variable final, and it's good to do so:
private final ConcurrentHashMap<String, ConcurrentHashMap<String, String>> session = new ConcurrentHashMap<String, ConcurrentHashMap<String, String>>();
You can simplify createSession a bit by ...
7
There is a bug in 'remove' method. A while loop never changes 'curr' object so in case when key is not equals then loop will never exit
7
You must not modify the keys of a HashMap while iterating over it. You are just lucky it worked.
Instead of the put(...), write e.setValue(e.getValue() + 1).
If you have some kind of Multiset available (e.g. when you are using Guava), you could replace your HashMap with a Multiset<String>, which will make your intention clearer.
Using lambdas, your ...
7
Why so slow
It's important to consider the time complexity of all the operations in your program.
Take a closer looks at this part I extracted from your code:
for(String s: strs)
{
// ...
pos.add(Arrays.asList(strs).indexOf(s));
For each s, converting a String[] to a List<String>, in order to use the indexOf method to find the index of s? ...
7
I don't see the need for unique_ptr for the root node.
spNode root;
I would just make this a node.
Node root;
The use of std::map is fine. But it does have O(log(n)) lookup. If you switch back to an array its O(1). Its a time for space thing. Pick the one you want.
I don't like the two line creation of nodes.
std::unique_ptr<Node> node(...
7
You should not use using namespace std; This is bad practice that will get you into trouble once you want to use a function that has the same name.
You can use descriptive names. So instead of "n" use "numEntries" or whatever.
You should declare the iteration variable inside the for loop rather than at the beginning.
std::map has an emplace method that you ...
7
void insert(const std::string &name)
{
auto it = data.find(name);
if (it == data.end())
{
data[name] = 1;
return;
}
data[name]++;
}
If it is available, you should prefer taking std::string_view parameters over std::string const&. You see, every time you do:
table.insert("Orange");
table.remove("Orange");
a ...
7
Just a few comments to add.
Data Structure
In this case, I'd tend to avoid std::map for counting frequencies. I probably wouldn't use std::unordered_map either though. Instead, I'd create a simple array:
std::array<int, std::numeric_limits<unsigned char>::max()> freq;
[Note: when using this, you want to convert the input characters to unsiged ...
Only top voted, non community-wiki answers of a minimum length are eligible
