Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##KeyValuePair

KeyValuePair

##HashTable

HashTable

##KeyValuePair

##HashTable

KeyValuePair

HashTable

Source Link
maaartinus
  • 13.7k
  • 1
  • 35
  • 74

As a substitute, I've started using Optionals, since its use makes it very clear whether or not the method can fail.

It tells you about the same as a Nullable annotation would do, just with some overhead.

There are a few cases in this code where a null would otherwise be used, but I opted for using an Optional. Are these cases a correct usage?

I personally agree with this wonderful rant that any use and the mere existence of Optional is wrong.

The underlying array is an ArrayList of Optionals of Pairs, since any given cell may or may not contain a value.

Too bad. That's not what Optional is for. It was meant to be used for return values only (that's why it's not Serializable).

And you added some 12 bytes per entry.

I decided to make my own pair class (KeyValuePair) to store the key-value-pair. Was this the correct approach to take?

Not exactly as there's already Map.Entry.

I figured passing in the hash-function as a lambda to the constructor of HashTable was the most flexible way of setting the function. Is my current set-up optimal?

It adds some overhead, but may make sense.

##KeyValuePair

Nothing to say, except for that usually toString would use "=" rather than ",". Just look at a HashMap in debugger.

An immutable KeyValuePair makes a lot of sense for an immutable map, but less for a mutable one. That's why Map.Entry allows to set the value (but not the key).

##HashTable

private final ArrayList<Optional<KeyValuePair<K, V>>> underArr;

By using an ArrayList (which should be declared as List) you're adding additional indirection. The same for Optional. You gain some flexibility, which you can't use.

  • with the size fixed, an array does the job
  • Optional buys you nothing at all

underArr is a terrible name. table would do.


//The user-supplied hash-function. It should take the key, and the size of
// the table, and return a hash.
private final BiFunction<K,Integer,Integer> hashFunc;

That's a pain. Letting the user specify a Function<K, Integer> so that they can choose how to hash their values may make sense. Letting them care about the table size is too bad.

Returning Integer rather than int (see IntFunction) means another efficiency loss.

public HashTable(int tableSize, BiFunction<K,Integer,Integer> f) {
 hashFunc = f;

So f or hashFunc? Both names are non-optimal (with f being terrible for anything but a local variable), using two different names is a sin.

 underArr = new ArrayList<Optional<KeyValuePair<K, V>>>(tableSize);
 initArr(tableSize);

Without Optional you could save yourself initArr (didn't I say, I hate the name?).


public boolean hasSpace() {
 for (Optional<KeyValuePair<K, V>> optPair : underArr) {
 if (!optPair.isPresent()) return true;
 }
 return false;
}

Do you really traverse the whole table on each insert? Otherwise, I can't see what this method is good for.

But if you traverse the whole table on each insert, then it's the final blow to any efficiency you could gain by hashing.

...it's getting too long and I'm getting too negative...

lang-java

AltStyle によって変換されたページ (->オリジナル) /