AbstractMap: cleanup and refactor

#KT-12386

js/js.libraries/src/core/collections/AbstractList.kt

@@ -93,27 +93,11 @@ public abstract class AbstractList<E> protected constructor() : AbstractCollecti
     override fun equals(other: Any?): Boolean {
         if (other === this) return true
         if (other !is List<*>) return false
-        if (size != other.size) return false
 
-        val otherIterator = other.iterator()
+        return orderedEquals(this, other)
-        for (elem in this) {
-            val elemOther = otherIterator.next()
-            if (elem != elemOther) {
-                return false
-            }
-        }
-
-        return true
     }
 
-    override fun hashCode(): Int {
+    override fun hashCode(): Int = orderedHashCode(this)
-        var hashCode = 1
-        for (e in this) {
-            hashCode = 31 * hashCode + (e?.hashCode() ?: 0)
-            hashCode = hashCode or 0 // make sure we don't overflow
-        }
-        return hashCode
-    }
 
 
     private open inner class IteratorImpl : MutableIterator<E> {
@@ -236,6 +220,28 @@ public abstract class AbstractList<E> protected constructor() : AbstractCollecti
                 throw IllegalArgumentException("fromIndex: $start > toIndex: $end")
             }
         }
+
+        internal fun orderedHashCode(c: Collection<*>): Int {
+            var hashCode = 1
+            for (e in c) {
+                hashCode = 31 * hashCode + (e?.hashCode() ?: 0)
+                hashCode = hashCode or 0 // make sure we don't overflow
+            }
+            return hashCode
+        }
+
+        internal fun orderedEquals(c: Collection<*>, other: Collection<*>): Boolean {
+            if (c.size != other.size) return false
+
+            val otherIterator = other.iterator()
+            for (elem in c) {
+                val elemOther = otherIterator.next()
+                if (elem != elemOther) {
+                    return false
+                }
+            }
+            return true
+        }
     }
 
 }

js/js.libraries/src/core/collections/AbstractMap.kt

@@ -25,88 +25,48 @@ abstract class AbstractMap<K, V> protected constructor() : MutableMap<K, V> {
     /**
      * A mutable [Map.Entry] shared by several [Map] implementations.
      */
-    open class SimpleEntry<K, V> : AbstractEntry<K, V> {
+    open class SimpleEntry<K, V>(override val key: K, value: V) : MutableMap.MutableEntry<K, V> {
-        constructor(key: K, value: V) : super(key, value) {
+        constructor(entry: Map.Entry<K, V>) : this(entry.key, entry.value)
+
+        private var _value = value
+
+        override val value: V get() = _value
+
+        override fun setValue(value: V): V {
+            val oldValue = this._value
+            this._value = value
+            return oldValue
         }
 
-        constructor(entry: Entry<out K, out V>) : super(entry.key, entry.value) {
+        override fun hashCode(): Int = entryHashCode(this)
-        }
+        override fun toString(): String = entryToString(this)
+        override fun equals(other: Any?): Boolean = entryEquals(this, other)
+
     }
 
     /**
      * An immutable [Map.Entry] shared by several [Map] implementations.
      */
-    class SimpleImmutableEntry<K, V> : AbstractEntry<K, V> {
+    class SimpleImmutableEntry<out K, out V>(override val key: K, override val value: V) : Map.Entry<K, V> {
-        constructor(key: K, value: V) : super(key, value) {
+        constructor(entry: Map.Entry<K, V>) : this(entry.key, entry.value)
-        }
 
-        constructor(entry: Entry<out K, out V>) : super(entry.key, entry.value) {
+        override fun hashCode(): Int = entryHashCode(this)
-        }
+        override fun toString(): String = entryToString(this)
-
+        override fun equals(other: Any?): Boolean = entryEquals(this, other)
-        override fun setValue(value: V): V {
-            throw UnsupportedOperationException()
-        }
     }
 
-    /**
-     * Basic [Map.Entry] implementation used by [SimpleEntry]
-     * and [SimpleImmutableEntry].
-     */
-    private abstract class AbstractEntry<K, V> protected constructor(private val key: K, private var value: V?) : Map.Entry<K, V> {
-
-        override fun getKey(): K {
-            return key
-        }
-
-        override fun getValue(): V {
-            return value
-        }
-
-        override fun setValue(value: V): V {
-            val oldValue = this.value
-            this.value = value
-            return oldValue
-        }
-
-        override fun equals(other: Any?): Boolean {
-            if (other !is Entry<*, *>) {
-                return false
-            }
-            val entry = other as Entry<*, *>?
-            return key == entry!!.key && value == entry!!.value
-        }
-
-        /**
-         * Calculate the hash code using Sun's specified algorithm.
-         */
-        override fun hashCode(): Int {
-            return Objects.hashCode(key) xor Objects.hashCode(value)
-        }
-
-        override fun toString(): String {
-            // for compatibility with the real Jre: issue 3422
-            return key + "=" + value
-        }
-    }
 
     override fun clear() {
         entries.clear()
     }
 
-    override fun containsKey(key: Any): Boolean {
+    override fun containsKey(key: K): Boolean {
         return implFindEntry(key, false) != null
     }
 
-    override fun containsValue(value: Any): Boolean {
+    override fun containsValue(value: V): Boolean = entries.any { it.value == value }
-        for ((key, v) in entries) {
-            if (value == v) {
-                return true
-            }
-        }
-        return false
-    }
 
-    internal fun containsEntry(entry: Entry<*, *>): Boolean {
+    internal fun containsEntry(entry: Map.Entry<*, *>): Boolean {
         val key = entry.key
         val value = entry.value
         val ourValue = get(key)
@@ -123,66 +83,40 @@ abstract class AbstractMap<K, V> protected constructor() : MutableMap<K, V> {
         return true
     }
 
-    override fun equals(obj: Any?): Boolean {
+    override fun equals(other: Any?): Boolean {
-        if (obj === this) {
+        if (other === this) return true
-            return true
+        if (other !is Map<*, *>) return false
-        }
+        if (size != other.size) return false
-        if (obj !is Map<*, *>) {
-            return false
-        }
-        if (size != obj.size) {
-            return false
-        }
 
-        for (entry in obj.entries) {
+        return other.entries.all { containsEntry(it) }
-            if (!containsEntry(entry)) {
-                return false
-            }
-        }
-        return true
     }
 
-    override operator fun get(key: Any): V? {
+    override operator fun get(key: K): V? = implFindEntry(key, false)?.value
-        return getEntryValueOrNull<K, V>(implFindEntry(key, false))
-    }
 
-    override fun hashCode(): Int {
+    override fun hashCode(): Int = entries.hashCode()
-        return Collections.hashCode(entries)
-    }
 
-    override fun isEmpty(): Boolean {
+    override fun isEmpty(): Boolean = size == 0
-        return size == 0
+    override val size: Int get() = entries.size
-    }
 
-    override fun keySet(): Set<K> {
+
+    override val keys: MutableSet<K> get() {
         return object : AbstractSet<K>() {
             override fun clear() {
                 this@AbstractMap.clear()
             }
 
-            override operator fun contains(key: Any?): Boolean {
+            override operator fun contains(key: K): Boolean = containsKey(key)
-                return containsKey(key)
-            }
 
-            override operator fun iterator(): Iterator<K> {
+            override operator fun iterator(): MutableIterator<K> {
                 val outerIter = entries.iterator()
-                return object : Iterator<K> {
+                return object : MutableIterator<K> {
-                    override fun hasNext(): Boolean {
+                    override fun hasNext(): Boolean = outerIter.hasNext()
-                        return outerIter.hasNext()
+                    override fun next(): K = outerIter.next().key
-                    }
+                    override fun remove() = outerIter.remove()
-
-                    override fun next(): K {
-                        val entry = outerIter.next()
-                        return entry.key
-                    }
-
-                    override fun remove() {
-                        outerIter.remove()
-                    }
                 }
             }
 
-            override fun remove(key: Any?): Boolean {
+            override fun remove(key: K): Boolean {
                 if (containsKey(key)) {
                     this@AbstractMap.remove(key)
                     return true
@@ -190,89 +124,63 @@ abstract class AbstractMap<K, V> protected constructor() : MutableMap<K, V> {
                 return false
             }
 
-            override fun size(): Int {
+            override val size: Int get() = this@AbstractMap.size
-                return this@AbstractMap.size
-            }
         }
     }
 
-    override fun put(key: K, value: V): V {
+    override fun put(key: K, value: V): V? {
         throw UnsupportedOperationException("Put not supported on this map")
     }
 
     override fun putAll(map: Map<out K, V>) {
-        checkNotNull(map)
         for ((key, value) in map) {
             put(key, value)
         }
     }
 
-    override fun remove(key: Any): V {
+    override fun remove(key: K): V? = implFindEntry(key, true)?.value
-        return getEntryValueOrNull<K, V>(implFindEntry(key, true))
-    }
 
-    override fun size(): Int {
+    override fun toString(): String = entries.joinToString(", ", "{", "}") { toString(it) }
-        return entries.size
-    }
 
-    override fun toString(): String {
+    private fun toString(entry: Map.Entry<K, V>): String = toString(entry.key) + "=" + toString(entry.value)
-        val joiner = StringJoiner(", ", "{", "}")
-        for (entry in entries) {
-            joiner.add(toString(entry))
-        }
-        return joiner.toString()
-    }
 
-    private fun toString(entry: Entry<K, V>): String {
+    private fun toString(o: Any?): String = if (o === this) "(this Map)" else o.toString()
-        return toString(entry.key) + "=" + toString(entry.value)
-    }
 
-    private fun toString(o: Any): String {
+    override val values: MutableCollection<V> get() {
-        return if (o === this) "(this Map)" else o.toString()
-    }
-
-    override fun values(): Collection<V> {
         return object : AbstractCollection<V>() {
-            override fun clear() {
+            override fun clear() = this@AbstractMap.clear()
-                this@AbstractMap.clear()
-            }
 
-            override operator fun contains(value: Any?): Boolean {
+            override operator fun contains(value: V): Boolean = containsValue(value)
-                return containsValue(value)
-            }
 
-            override operator fun iterator(): Iterator<V> {
+            override operator fun iterator(): MutableIterator<V> {
                 val outerIter = entries.iterator()
-                return object : Iterator<V> {
+                return object : MutableIterator<V> {
-                    override fun hasNext(): Boolean {
+                    override fun hasNext(): Boolean = outerIter.hasNext()
-                        return outerIter.hasNext()
+                    override fun next(): V = outerIter.next().value
-                    }
+                    override fun remove() = outerIter.remove()
-
-                    override fun next(): V {
-                        val entry = outerIter.next()
-                        return entry.value
-                    }
-
-                    override fun remove() {
-                        outerIter.remove()
-                    }
                 }
             }
 
-            override fun size(): Int {
+            override val size: Int get() = this@AbstractMap.size
-                return this@AbstractMap.size
+
+            // TODO: should we implement them this way? Currently it's unspecified in JVM
+            override fun equals(other: Any?): Boolean {
+                if (this === other) return true
+                if (other !is Collection<*>) return false
+                return AbstractList.orderedEquals(this, other)
             }
+            override fun hashCode(): Int = AbstractList.orderedHashCode(this)
         }
     }
 
-    private fun implFindEntry(key: Any, remove: Boolean): Entry<K, V>? {
+    private fun implFindEntry(key: K, remove: Boolean): Map.Entry<K, V>? {
         val iter = entries.iterator()
         while (iter.hasNext()) {
             var entry = iter.next()
             val k = entry.key
             if (key == k) {
                 if (remove) {
-                    entry = SimpleEntry(entry.key, entry.value)
+                    entry = SimpleEntry(entry)
                     iter.remove()
                 }
                 return entry
@@ -283,12 +191,11 @@ abstract class AbstractMap<K, V> protected constructor() : MutableMap<K, V> {
 
     companion object {
 
-        internal fun <K, V> getEntryKeyOrNull(entry: Entry<K, V>?): K? {
+        internal fun entryHashCode(e: Map.Entry<*, *>): Int = with(e) { (key?.hashCode() ?: 0) xor (value?.hashCode() ?: 0) }
-            return if (entry == null) null else entry!!.key
+        internal fun entryToString(e: Map.Entry<*, *>): String = with(e) { "$key=$value" }
-        }
+        internal fun entryEquals(e: Map.Entry<*, *>, other: Any?): Boolean {
-
+            if (other !is Map.Entry<*, *>) return false
-        internal fun <K, V> getEntryValueOrNull(entry: Entry<K, V>?): V? {
+            return e.key == other.key && e.value == other.value
-            return if (entry == null) null else entry!!.value
         }
     }
 }