14

TL;DR:

How do I make this less redundant (any approach that works helps)?

if (personModification.firstName != null) {person.firstName = personModification.firstName}
if (personModification.lastName != null) {person.lastName = personModification.lastName}
if (personModification.job != null) {person.job = personModification.job}

The long version: I have a simple problem. I have a class Person:

class Person (val firstName: String?, 
              val lastName: String?, 
              val job: String?)

and I have a class called PersonModification:

class PersonModification(val firstName: String?, 
                         val lastName: String?, 
                         val job: String?)

The task is to overwrite any Person property values with PersonModification values, IF the PersonModification property isn't null. If you care, the business logic behind this is an API endpoint which modifies Person and takes a PersonModification as an argument (but can change all, or any, of the properties, so we don't want to overwrite valid old values with nulls). The solution to this looks like this.

if (personModification.firstName != null) {person.firstName = personModification.firstName}
if (personModification.lastName != null) {person.lastName = personModification.lastName}
if (personModification.job != null) {person.job = personModification.job}

I was told this is redundant (and I agree). The solution pseudocode looks like this:

foreach(propName in personProps){
  if (personModification["propName"] != null) {person["propName"] = personModification["propName"]}
}

Of course, this isn't JavaScript, so it's not that easy. My reflection solution is below, but imo, it's better to have redundancy than do reflection here. What are my other options to remove the redundancy?


Refelection:

package kotlin.reflect;

class Person (val firstName: String?, 
              val lastName: String?, 
              val job: String?)

class PersonModification(val firstName: String?, 
                         val lastName: String?, 
                         val job: String?)

// Reflection - a bad solution. Impossible without it.
//https://stackoverflow.com/questions/35525122/kotlin-data-class-how-to-read-the-value-of-property-if-i-dont-know-its-name-at
inline fun <reified T : Any> Any.getThroughReflection(propertyName: String): T? {
    val getterName = "get" + propertyName.capitalize()
    return try {
        javaClass.getMethod(getterName).invoke(this) as? T
    } catch (e: NoSuchMethodException) {
        null
    }
}

fun main(args: Array<String>) {

var person: Person = Person("Bob","Dylan","Artist")
val personModification: PersonModification = PersonModification("Jane","Smith","Placeholder")
val personClassPropertyNames = listOf("firstName", "lastName", "job")

for(properyName in personClassPropertyNames) {
    println(properyName)
    val currentValue = person.getThroughReflection<String>(properyName)
    val modifiedValue = personModification.getThroughReflection<String>(properyName)
    println(currentValue)
    if(modifiedValue != null){
        //Some packages or imports are missing for "output" and "it"
        val property = outputs::class.memberProperties.find { it.name == "firstName" }
        if (property is KMutableProperty<*>) {
            property.setter.call(person, "123")
        }
    }
})
}

You can copy and paste here to run it: https://try.kotlinlang.org/

VSO
  • 11,546
  • 25
  • 99
  • 187
  • 3
    It doesn't remove your redundancy, but you could drop the null check by falling back to your old value if null, e.g. `person.firstName = personModification.firstName ?: person.firstName` – Cailean Wilkinson Jul 18 '18 at 00:56
  • Thanks, I was trying a ternary, and when I realized Kotlin doesn't have it, I didn't think to use null coalescing. – VSO Jul 18 '18 at 01:02
  • 2
    check the relevant question https://stackoverflow.com/questions/39199426/better-way-to-map-kotlin-data-objects-to-data-objects. may be it can give you more idea – Linh Jul 18 '18 at 01:30
  • "I was trying a ternary, and when I realized Kotlin doesn't have it" it does, it's just called `if`. Though in this case `?:` is indeed better, of course. – Alexey Romanov Jul 18 '18 at 09:12

7 Answers7

7

It should be pretty simple to write a 5 line helper to do this which even supports copying every matching property or just a selection of properties.

Although it's probably not useful if you're writing Kotlin code and heavily utilising data classes and val (immutable properties). Check it out:

fun <T : Any, R : Any> T.copyPropsFrom(fromObject: R, skipNulls: Boolean = true, vararg props: KProperty<*>) {
  // only consider mutable properties
  val mutableProps = this::class.memberProperties.filterIsInstance<KMutableProperty<*>>()
  // if source list is provided use that otherwise use all available properties
  val sourceProps = if (props.isEmpty()) fromObject::class.memberProperties else props.toList()
  // copy all matching
  mutableProps.forEach { targetProp ->
    sourceProps.find {
      // make sure properties have same name and compatible types 
      it.name == targetProp.name && targetProp.returnType.isSupertypeOf(it.returnType) 
    }?.let { matchingProp ->
      val copyValue = matchingProp.getter.call(fromObject);
      if (!skipNulls || (skipNulls && copyValue != null)) {
        targetProp.setter.call(this, copyValue)
      }
    }
  }
}

This approach uses reflection, but it uses Kotlin reflection which is very lightweight. I haven't timed anything, but it should run almost at same speed as copying properties by hand.

Also it uses KProperty instead of strings to define a subset of properties (if you don't want all of them copied) so it has complete refactoring support, so if you rename a property on the class you won't have to hunt for string references to rename.

It will skip nulls by default or you can toggle the skipNulls parameters to false (default is true).

Now given 2 classes:

data class DataOne(val propA: String, val propB: String)
data class DataTwo(var propA: String = "", var propB: String = "")

You can do the following:

  var data2 = DataTwo()
  var data1 = DataOne("a", "b")
  println("Before")
  println(data1)
  println(data2)
  // this copies all matching properties
  data2.copyPropsFrom(data1)
  println("After")
  println(data1)
  println(data2)
  data2 = DataTwo()
  data1 = DataOne("a", "b")
  println("Before")
  println(data1)
  println(data2)
  // this copies only matching properties from the provided list 
  // with complete refactoring and completion support
  data2.copyPropsFrom(data1, DataOne::propA)
  println("After")
  println(data1)
  println(data2)

Output will be:

Before
DataOne(propA=a, propB=b)
DataTwo(propA=, propB=)
After
DataOne(propA=a, propB=b)
DataTwo(propA=a, propB=b)
Before
DataOne(propA=a, propB=b)
DataTwo(propA=, propB=)
After
DataOne(propA=a, propB=b)
DataTwo(propA=a, propB=)
Strelok
  • 50,229
  • 9
  • 102
  • 115
  • There was a small bug in the code. Check out the updated answer. – Strelok Jul 18 '18 at 12:11
  • 1
    This is nor the simplest nor the shortest answer and it is also buggy. I don't get why this is the accepted answer. – Adam Arold Jul 26 '18 at 14:37
  • Yeah, there seems to be an error somewhere. Played around with it a bit, but don't want to go into that method ;-) I can only warn again: before implementing your own model mapper, use something that is proven and developed by lots of others, which at least already fixed the most obvious bugs ;-) I also wonder, why that answer got so many upvotes. The people may not have tested it really. – Roland Jul 26 '18 at 15:05
  • By the way: it seems to be rather slow. Did not do real performance tests though. Just some basic measurements. Definetely not something you want to have in a mapping utility for DTOs. – Roland Jul 26 '18 at 15:10
  • OK, only the first reflection call is really slow. Thereafter it's ok. – Roland Jul 26 '18 at 15:14
2

This can be solved without reflection using delegated properties. See: https://kotlinlang.org/docs/reference/delegated-properties.html

class Person(firstName: String?,
             lastName: String?,
             job: String?) {
    val map = mutableMapOf<String, Any?>()
    var firstName: String? by map
    var lastName: String? by map
    var job: String? by map

    init {
        this.firstName = firstName
        this.lastName = lastName
        this.job = job
    }
}

class PersonModification(firstName: String?,
                         lastName: String?,
                         job: String?) {
    val map = mutableMapOf<String, Any?>()
    var firstName: String? by map
    var lastName: String? by map
    var job: String? by map

    init {
        this.firstName = firstName
        this.lastName = lastName
        this.job = job
    }
}


fun main(args: Array<String>) {

    val person = Person("Bob", "Dylan", "Artist")
    val personModification1 = PersonModification("Jane", "Smith", "Placeholder")
    val personModification2 = PersonModification(null, "Mueller", null)

    println("Person: firstName: ${person.firstName}, lastName: ${person.lastName}, job: ${person.job}")

    personModification1.map.entries.forEach { entry -> if (entry.value != null) person.map[entry.key] = entry.value }
    println("Person: firstName: ${person.firstName}, lastName: ${person.lastName}, job: ${person.job}")

    personModification2.map.entries.forEach { entry -> if (entry.value != null) person.map[entry.key] = entry.value }
    println("Person: firstName: ${person.firstName}, lastName: ${person.lastName}, job: ${person.job}")


}
Alexander Egger
  • 5,132
  • 1
  • 28
  • 42
2

You can create a nice trait for this which you will be able to apply for any modification class you might have:

interface Updatable<T : Any> {

    fun updateFrom(model: T) {
        model::class.java.declaredFields.forEach { modelField ->
            this::class.java.declaredFields
                    .filter { it.name == modelField.name && it.type == modelField.type }
                    .forEach { field ->
                        field.isAccessible = true
                        modelField.isAccessible = true
                        modelField.get(model)?.let { value ->
                            field.set(this, value)
                        }
                    }
        }
    }
}

Usage:

data class Person(val firstName: String?,
                  val lastName: String?,
                  val job: String?) : Updatable<PersonModification>

data class PersonModification(val firstName: String?,
                              val lastName: String?,
                              val job: String?)

Then you can try it out:

fun main(args: Array<String>) {

    val person = Person(null, null, null)

    val mod0 = PersonModification("John", null, null)
    val mod1 = PersonModification(null, "Doe", null)
    val mod2 = PersonModification(null, null, "Unemployed")

    person.updateFrom(mod0)
    println(person)

    person.updateFrom(mod1)
    println(person)

    person.updateFrom(mod2)
    println(person)
}

This will print:

Person(firstName=John, lastName=null, job=null)
Person(firstName=John, lastName=Doe, job=null)
Person(firstName=John, lastName=Doe, job=Unemployed)
Adam Arold
  • 29,285
  • 22
  • 112
  • 207
2

model mapping utilities

You can also use one of the many model mapping utilities, like the ones listed in http://www.baeldung.com/java-performance-mapping-frameworks (there at least you already see some performance benchmarks regarding the different kind of model mappers).

Note that I cannot really recommend writing your own mapping utility if you do not test it thoroughly. Already seen examples where the custom mapping utility grew and grew and later on lead to strange behaviour as some corner cases weren't considered.

simplifying the != null

Otherwise, if you are not too lazy, I would rather recommend something like:

personModification.firstName?.also { person.firstName = it }

It doesn't require any reflection, is simple and still readable... somehow at least ;-)

delegated properties

Another thing that comes to my mind and somehow matches your Javascript approach are delegated properties (which I only recommend if the backed Map is a suitable model for you; actually what I am showing below is rather a delegated person map using a HashMap, which I can not really recommend, but which is quite an easy and useful way to get the Javascript look&feel; the reason why I don't recommend it: is Person a Map? ;-)).

class Person() : MutableMap<String, String?> by HashMap() { // alternatively use class Person(val personProps : MutableMap<String, String?> = HashMap()) instead and replace `this` below with personProps
  var firstName by this
  var lastName by this
  var job by this
  constructor(firstName : String?, lastName : String?, job : String?) : this() {
    this.firstName = firstName
    this.lastName = lastName
    this.job = job
  }
}

The PersonModification-class then basically looks the same. Applying the mapping would then look like:

val person = Person("first", "last", null)
val personMod = PersonModification("new first", null, "new job")
personMod.filterValues { it != null }
        .forEach { key, value -> person[key] = value } // here the benefit of extending the Map becomes visible: person[key] instead of person.personProps[key], but then again: person.personProps[key] is cleaner

If you do not require that secondary constructor it's even better, then the class looks nearly as before and the properties can be set and get as before.

Thinking about it you do not really need the secondary constructor as you could still use apply and then just add the variables you are interested in (nearly as named parameters). Then the class would look similar to:

class PersonModification : MutableMap<String, String?> by HashMap() { // or again simply: class PersonModification(props : MutableMap<String, String?> = HashMap()) and replacing `this` with props below
  var firstName by this
  var lastName by this
  var job by this
}

and instantiating it then looks as follows:

val personMod = PersonModification().apply {
    firstName = "new first"
    job = "new job"
}

Mapping would still be the same.

Roland
  • 22,259
  • 4
  • 57
  • 84
  • Wow, that's a cool approach. I did not know that you can delegate properties to a `Map`. Can you point me to some more detailed info on how this works? I'm intrigued! – Adam Arold Jul 26 '18 at 14:39
  • Regarding the map delegation the following will point you directly to the documentation: https://kotlinlang.org/docs/reference/delegated-properties.html#storing-properties-in-a-map Regarding the class delegation to the `HashMap` I can't really recommend it, but it's a simple way to also access the properties as easily as in Javascript and it's ok if you don't mind that your class extends `Map` in the first place (which I really really do not recommend ;-)). – Roland Jul 26 '18 at 14:46
1

Already many people offered their solutions. But I want to offer one more:

There are interesting feature in jackson, you could try to merge json. So, you could merge src object with deserialization version of PersonModification

With it, it's possible to do something like this:

class ModificationTest {
    @Test
    fun test() {
        val objectMapper = jacksonObjectMapper().apply {
            setSerializationInclusion(JsonInclude.Include.NON_NULL)
        }

        fun Person.merge(personModification: PersonModification): Person = run {
            val temp = objectMapper.writeValueAsString(personModification)

            objectMapper.readerForUpdating(this).readValue(temp)
        }

        val simplePerson = Person("firstName", "lastName", "job")

        val modification = PersonModification(firstName = "one_modified")
        val modification2 = PersonModification(lastName = "lastName_modified")

        val personAfterModification1: Person = simplePerson.merge(modification)
        //Person(firstName=one_modified, lastName=lastName, job=job)
        println(personAfterModification1)

        val personAfterModification2: Person = personAfterModification1.merge(modification2)
        //Person(firstName=one_modified, lastName=lastName_modified, job=job)
        println(personAfterModification2)
    }
}

Hope this will help you!

kurt
  • 1,510
  • 2
  • 13
  • 23
0

Create an extension function for Person:

fun Person.modify(pm: PersonModification) {
    pm.firstName?.let { firstName = it }
    pm.lastName?.let { lastName = it }
    pm.job?.let { job = it }
}

fun Person.println() {
    println("firstName=$firstName, lastName=$lastName, job=$job")
}

and use it like this:

fun main(args: Array <String> ) {
    val p = Person("Nick", "Doe", "Cartoonist")
    print("Person before: ")
    p.println()

    val pm = PersonModification("Maria", null, "Actress")
    p.modify(pm)

    print("Person after: ")
    p.println()
}

Or choose one of the following:

fun Person.println() {
    println("firstName=$firstName, lastName=$lastName, job=$job")
}

fun main(args: Array <String> ) {
    val p = Person("Nick", "Doe", "Cartoonist")
    print("Person before: ")
    p.println()

    val pm = PersonModification("John", null, null)

    pm.firstName?.run { p.firstName = this }.also { pm.lastName?.run { p.lastName = this } }.also { pm.job?.run { p.job = this } }
    // or
    pm.firstName?.also { p.firstName = it }.also { pm.lastName?.also { p.lastName = it } }.also { pm.job?.also { p.job = it } }
    // or 
    with (pm) {
        firstName?.run { p.firstName = this }
        lastName?.run { p.lastName= this }
        job?.run { p.job= this }
    }

    print("Person after: ")
    p.println()
}
0

It is nothing fancy, but it hides the complexity of mutating Person from the outside world.

class Person(
        var firstName: String?,
        var lastName: String?,
        var job: String?
) {
    fun modify(p: PersonModification){
        p.firstName?.let { firstName = it }
        p.lastName?.let { lastName = it }
        p.job?.let { job = it }
    }
}

class PersonModification(/* ... */)
Willi Mentzel
  • 27,862
  • 20
  • 113
  • 121