chore(library): Port Feature to Kotlin and enhance tests

dkhawk · dkhawk · commit 449258dfddc2 · 2025-08-29T10:49:24.000-06:00
This commit modernizes the `Feature` class by porting it from Java to idiomatic Kotlin, improving its conciseness, readability, and null safety.

The key changes include:
- **Porting `Feature` to Kotlin**: The class is now written in Kotlin, using modern language features like properties and a primary constructor.
- **Correct Observable Behavior**: Setters for geometry and properties now correctly call `setChanged()` and `notifyObservers()`, ensuring that observers are notified of modifications.
- **Test Modernization**: The corresponding `FeatureTest` has been ported to Kotlin and now uses Google Truth for more expressive assertions.
- **Enhanced Test Coverage**: New tests have been added to verify the `Observable` behavior when a feature's properties or geometry are changed.
- **Subclass Update**: `GeoJsonFeature` has been updated to align with the changes in its `Feature` superclass.
diff --git a/library/src/main/java/com/google/maps/android/data/Feature.kt b/library/src/main/java/com/google/maps/android/data/Feature.kt
@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Google Inc.
+ * Copyright 2025 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -13,117 +13,73 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+package com.google.maps.android.data
 
-package com.google.maps.android.data;
-
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Observable;
+import java.util.Observable
 
 /**
  * An abstraction that shares the common properties of
- * {@link com.google.maps.android.data.kml.KmlPlacemark KmlPlacemark} and
- * {@link com.google.maps.android.data.geojson.GeoJsonFeature GeoJsonFeature}
+ * [com.google.maps.android.data.kml.KmlPlacemark] and
+ * [com.google.maps.android.data.geojson.GeoJsonFeature]
  */
-public class Feature extends Observable {
-
-    protected String mId;
-
-    private final Map<String, String> mProperties;
-
-    private Geometry mGeometry;
-
-    /**
-     * Creates a new Feature object
-     *
-     * @param featureGeometry type of geometry to assign to the feature
-     * @param id              common identifier of the feature
-     * @param properties      map containing properties related to the feature
-     */
-    public Feature(Geometry featureGeometry, String id,
-                   Map<String, String> properties) {
-        mGeometry = featureGeometry;
-        mId = id;
-        if (properties == null) {
-            mProperties = new HashMap<>();
-        } else {
-            mProperties = properties;
+open class Feature(
+    geometry: Geometry<*>?,
+    id: String?,
+    properties: Map<String, String>?
+) : Observable() {
+    open var id: String? = id
+        protected set
+
+    private val _properties: MutableMap<String, String> = properties?.toMutableMap() ?: mutableMapOf()
+
+    open var geometry: Geometry<*>? = geometry
+        protected set(value) {
+            field = value
+            setChanged()
+            notifyObservers()
         }
-    }
 
     /**
      * Returns all the stored property keys
-     *
-     * @return iterable of property keys
      */
-    public Iterable<String> getPropertyKeys() {
-        return mProperties.keySet();
-    }
+    val propertyKeys: Iterable<String>
+        get() = _properties.keys
 
     /**
      * Gets the property entry set
-     *
-     * @return property entry set
      */
-    public Iterable getProperties() {
-        return mProperties.entrySet();
-    }
+    val properties: Iterable<Map.Entry<String, String>>
+        get() = _properties.entries
 
     /**
      * Gets the value for a stored property
      *
      * @param property key of the property
      * @return value of the property if its key exists, otherwise null
      */
-    public String getProperty(String property) {
-        return mProperties.get(property);
-    }
-
-    /**
-     * Gets the id of the feature
-     *
-     * @return id
-     */
-    public String getId() {
-        return mId;
-    }
+    fun getProperty(property: String): String? = _properties[property]
 
     /**
      * Checks whether the given property key exists
      *
      * @param property key of the property to check
      * @return true if property key exists, false otherwise
      */
-    public boolean hasProperty(String property) {
-        return mProperties.containsKey(property);
-    }
-
-    /**
-     * Gets the geometry object
-     *
-     * @return geometry object
-     */
-    public Geometry getGeometry() {
-        return mGeometry;
-    }
+    fun hasProperty(property: String): Boolean = _properties.containsKey(property)
 
     /**
      * Gets whether the placemark has properties
      *
      * @return true if there are properties in the properties map, false otherwise
      */
-    public boolean hasProperties() {
-        return mProperties.size() > 0;
-    }
+    fun hasProperties(): Boolean = _properties.isNotEmpty()
 
     /**
      * Checks if the geometry is assigned
      *
      * @return true if feature contains geometry object, otherwise null
      */
-    public boolean hasGeometry() {
-        return (mGeometry != null);
-    }
+    fun hasGeometry(): Boolean = geometry != null
 
     /**
      * Store a new property key and value
@@ -132,8 +88,11 @@ public class Feature extends Observable {
      * @param propertyValue value of the property to store
      * @return previous value with the same key, otherwise null if the key didn't exist
      */
-    protected String setProperty(String property, String propertyValue) {
-        return mProperties.put(property, propertyValue);
+    protected open fun setProperty(property: String, propertyValue: String): String? {
+        val prev = _properties.put(property, propertyValue)
+        setChanged()
+        notifyObservers()
+        return prev
     }
 
     /**
@@ -142,16 +101,10 @@ public class Feature extends Observable {
      * @param property key of the property to remove
      * @return value of the removed property or null if there was no corresponding key
      */
-    protected String removeProperty(String property) {
-        return mProperties.remove(property);
-    }
-
-    /**
-     * Sets the stored Geometry and redraws it on the layer if it has already been added
-     *
-     * @param geometry Geometry to set
-     */
-    protected void setGeometry(Geometry geometry) {
-        mGeometry = geometry;
+    protected open fun removeProperty(property: String): String? {
+        val prev = _properties.remove(property)
+        setChanged()
+        notifyObservers()
+        return prev
     }
 }
diff --git a/library/src/main/java/com/google/maps/android/data/geojson/GeoJsonFeature.java b/library/src/main/java/com/google/maps/android/data/geojson/GeoJsonFeature.java
@@ -54,7 +54,6 @@ public class GeoJsonFeature extends Feature implements Observer {
     public GeoJsonFeature(Geometry geometry, String id,
                           HashMap<String, String> properties, LatLngBounds boundingBox) {
         super(geometry, id, properties);
-        mId = id;
         mBoundingBox = boundingBox;
     }
 
@@ -238,7 +237,7 @@ public String toString() {
         sb.append(",\n point style=").append(mPointStyle);
         sb.append(",\n line string style=").append(mLineStringStyle);
         sb.append(",\n polygon style=").append(mPolygonStyle);
-        sb.append(",\n id=").append(mId);
+        sb.append(",\n id=").append(getId());
         sb.append(",\n properties=").append(getProperties());
         sb.append("\n}\n");
         return sb.toString();
diff --git a/library/src/test/java/com/google/maps/android/data/FeatureTest.kt b/library/src/test/java/com/google/maps/android/data/FeatureTest.kt
@@ -1,78 +1,131 @@
 /*
- * Copyright 2020 Google Inc.
- * 
+ * Copyright 2025 Google LLC
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
- *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package com.google.maps.android.data;
+package com.google.maps.android.data
 
-import com.google.android.gms.maps.model.LatLng;
+import com.google.android.gms.maps.model.LatLng
+import com.google.common.truth.Truth.assertThat
+import org.junit.Test
+import java.util.Observable
+import java.util.Observer
 
-import org.junit.Test;
+class FeatureTest {
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.Map;
+    // Test subclass to access protected members
+    private class TestFeature(
+        geometry: Geometry<*>?,
+        id: String?,
+        properties: Map<String, String>?
+    ) : Feature(geometry, id, properties) {
+        public override var id: String?
+            get() = super.id
+            set(value) {
+                super.id = value
+            }
 
-import static org.junit.Assert.*;
+        public override var geometry: Geometry<*>?
+            get() = super.geometry
+            set(value) {
+                super.geometry = value
+            }
+
+        public override fun setProperty(property: String, propertyValue: String): String? {
+            return super.setProperty(property, propertyValue)
+        }
+
+        public override fun removeProperty(property: String): String? {
+            return super.removeProperty(property)
+        }
+    }
+
+    @Test
+    fun `getId returns correct id`() {
+        var feature: Feature = Feature(null, "Pirate", null)
+        assertThat(feature.id).isEqualTo("Pirate")
+        feature = Feature(null, null, null)
+        assertThat(feature.id).isNull()
+    }
+
+    @Test
+    fun `properties work as expected`() {
+        val properties = mapOf("Color" to "Red", "Width" to "3")
+        val feature = Feature(null, null, properties)
+
+        assertThat(feature.hasProperty("llama")).isFalse()
+        assertThat(feature.hasProperty("Color")).isTrue()
+        assertThat(feature.getProperty("Color")).isEqualTo("Red")
+        assertThat(feature.hasProperties()).isTrue()
+        assertThat(feature.propertyKeys).containsExactly("Color", "Width")
+    }
+
+    @Test
+    fun `protected property methods work as expected`() {
+        val testFeature = TestFeature(null, null, mutableMapOf("Color" to "Red", "Width" to "3"))
+
+        assertThat(testFeature.removeProperty("Width")).isEqualTo("3")
+        assertThat(testFeature.hasProperty("Width")).isFalse()
+
+        assertThat(testFeature.setProperty("Width", "10")).isNull()
+        assertThat(testFeature.getProperty("Width")).isEqualTo("10")
+
+        assertThat(testFeature.setProperty("Width", "500")).isEqualTo("10")
+        assertThat(testFeature.getProperty("Width")).isEqualTo("500")
+    }
 
-public class FeatureTest {
     @Test
-    public void testGetId() {
-        Feature feature = new Feature(null, "Pirate", null);
-        assertNotNull(feature.getId());
-        assertEquals("Pirate", feature.getId());
-        feature = new Feature(null, null, null);
-        assertNull(feature.getId());
+    fun `geometry works as expected`() {
+        val feature = Feature(null, null, null)
+        assertThat(feature.hasGeometry()).isFalse()
+        assertThat(feature.geometry).isNull()
+
+        val point = Point(LatLng(0.0, 0.0))
+        val featureWithPoint = Feature(point, null, null)
+        assertThat(featureWithPoint.hasGeometry()).isTrue()
+        assertThat(featureWithPoint.geometry).isEqualTo(point)
     }
 
     @Test
-    public void testProperty() {
-        Map<String, String> properties = new HashMap<>();
-        properties.put("Color", "Red");
-        properties.put("Width", "3");
-        Feature feature = new Feature(null, null, properties);
-        assertFalse(feature.hasProperty("llama"));
-        assertTrue(feature.hasProperty("Color"));
-        assertEquals("Red", feature.getProperty("Color"));
-        assertTrue(feature.hasProperty("Width"));
-        assertEquals("3", feature.getProperty("Width"));
-        assertNull(feature.removeProperty("banana"));
-        assertEquals("3", feature.removeProperty("Width"));
-        assertNull(feature.setProperty("Width", "10"));
-        assertEquals("10", feature.setProperty("Width", "500"));
+    fun `protected geometry setter works`() {
+        val testFeature = TestFeature(null, null, null)
+        val point = Point(LatLng(0.0, 0.0))
+        testFeature.geometry = point
+        assertThat(testFeature.geometry).isEqualTo(point)
     }
 
     @Test
-    public void testGeometry() {
-        Feature feature = new Feature(null, null, null);
-        assertNull(feature.getGeometry());
-        Point point = new Point(new LatLng(0, 0));
-        feature.setGeometry(point);
-        assertEquals(point, feature.getGeometry());
-        feature.setGeometry(null);
-        assertNull(feature.getGeometry());
-
-        LineString lineString =
-                new LineString(
-                        new ArrayList<>(Arrays.asList(new LatLng(0, 0), new LatLng(50, 50))));
-        feature = new Feature(lineString, null, null);
-        assertEquals(lineString, feature.getGeometry());
-        feature.setGeometry(point);
-        assertEquals(point, feature.getGeometry());
-        feature.setGeometry(null);
-        assertNull(feature.getGeometry());
-        feature.setGeometry(lineString);
-        assertEquals(lineString, feature.getGeometry());
+    fun `observable notifies on change`() {
+        val feature = TestFeature(null, null, null)
+        val observer = TestObserver()
+        feature.addObserver(observer)
+
+        feature.setProperty("key", "value")
+        assertThat(observer.wasUpdated).isTrue()
+        observer.wasUpdated = false // reset
+
+        feature.removeProperty("key")
+        assertThat(observer.wasUpdated).isTrue()
+        observer.wasUpdated = false // reset
+
+        feature.geometry = Point(LatLng(1.0, 1.0))
+        assertThat(observer.wasUpdated).isTrue()
+    }
+
+    class TestObserver : Observer {
+        var wasUpdated = false
+        override fun update(o: Observable?, arg: Any?) {
+            wasUpdated = true
+        }
     }
-}
+}
