I’m doing some clean up on our code base to elimin...

# getting-started

[Larry Garfield]

I’m doing some clean up on our code base to eliminate compiler warnings. I’m getting a warning on this:

updatedDto.registeredAgent?.let { ra ->
  it.agentType = updatedEntity.agentType
  it.agentName = updatedEntity.agentName
  // .. A bunch more lines like this
}

That ra is unused. Which, yes, true. But what I don’t understand is how the code even works. If ra is defined, doesn’t that replace it as the variable? Why is it even valid at that point?

[Larry Garfield]

(Code was written by someone no longer on the team, it seems, so no clue why it is what it is.)

[Chris Lee]

it is from some higher scope, not evident in that code snippet. Its questionable naming - that higher-level it should be called something meaningful to avoid confusion with inner scopes.
Instead of ?.let with an unused it it may be cleaner to use ?.run

[Larry Garfield]

Oh geez, so it is. This whole function is a mess. 🙂

[Larry Garfield]

How does let differ from any? (I’m still new and haven’t fully grokked the different applicator functions.)

[Chris Lee]

All the scope functions are documented here. let and run are similar, the difference being the object reference (let has it, run does not). I generally use let in a similar way to .map on a collection, to take some object and transform it. For your code snippet, where it appears the goal is to execute a block of code if there is a non-null registered agent, run is cleaner (it doesn’t leave you with the unused object reference). If you were using the object reference also would be an alternative (you can still use that, with _ as the object reference name). Note that run and let return the value of the lambda - also return the context object (updateDto.registeredAgent in your case).

[Larry Garfield]

Hm. So in this particular case, just swapping let to run would work. In the general case, I think this all needs to get converted to an extension function anyway.

[Chris Lee]

yea, usually need to look at the overall context to decide on the best patterns to apply.

[Larry Garfield]

The general structure here is:

fun updateById(id, updatedDto) {
  val updatedEntity = updatedDto.toEntity()
  return repo.findById(id).map {
    // A whole bunch of lines with bad null handling to 
    // map stuff from updatedEntity to it.
    updatedDto.subObjA?.let { a -> 
       it.field1 = updatedEntity.field1
       // ...
    }
    // The same block repeated for a few other fields.

    return@map repo.save(it).toDto()
  }
}

Which is… horrifying, to be clear. For many reasons, I’m sure.

[Klitos Kyriacou]

In this particular case, I would find this more readable:

if (updatedDto.subObjA != null) {
       it.field1 = updatedEntity.field1
       // ...
    }

It makes it clear that the block is only executed if that object is not null, and that we are not actually using that object inside the block.

Also, that return@map at the end is a smell.

[Chris Lee]

oh. wow. yea.
An alternate structure:

fun updateById(id, updatedDto) {
  val updatedEntity = updatedDto.toEntity()
  val currentEntity = repo.findById(id) ?: return // or perhaps some form of error, unclear how the existing code works for "not found"
  if(updatedDto.subObjA != null) {
    currentEntity.field1 = updatedEntity.field1
    // ...
  }
  return repo.save(currentEntity).toDto()
}

[Larry Garfield]

I was going to go with an extension function:

fun CurrentEntityType.updateFrom(updatedEntity): CurrentEntityType {
  updatedEntity.fieldA?.let { fieldA = it }
  // ...
}

(I did that elsewhere in this app and it seemed to work pretty well.)

And then something similar for the inner blocks.

[Chris Lee]

Yep, that would help extract the logic.

fun updateById(id, updatedDto) {
  val updatedEntity = updatedDto.toEntity()
  val currentEntity = repo.findById(id).single() // or perhaps some form of error, unclear how the existing code works for "not found"
  currentEntity.updateFrom(updatedEntity)
  return repo.save(currentEntity).toDto()
}

fun CurrentEntityType.updateFrom(updatedEntity): CurrentEntityType {
  fieldA = updatedEntity.fieldA // this overwrites in all cases, including nulls
  updatedEntity.fieldA?.let { fieldA = it }  // this only overwrites when there are incoming non-null values
  // ...
}

[Larry Garfield]

(I’m finding I really like extension functions for “data translation” cases.)

[Larry Garfield]

OK, I’ll see about doing that refactor at some point.

[Chris Lee]

(I’m finding I really like extension functions for “data translation” cases.)

Yes, that’s a great use case for them - you can add them externally to the objects so they are in-context of whatever module you are working on without the main object knowing about all that stuff.

[CLOVIS]

Few thoughts looking at this code:
• The map is mutating the contents of what it is mapping. I don't like this, map should be pure transformation, where the source of the map is not changed.
• I guess they used map because they want to return the result of the DTO mapping at the end, but there's a lot of other stuff going to and it obscures the save .
I really don't like that objects are mutable. If I had the option, I would make them all read-only. If I couldn't do anything about it, I'd write:

fun updateById(id, updatedDto) {
  val updatedEntity = updatedDto.toEntity()
  return repo.findById(id)
      .onEach {
          // …
          if (updatedDto.subObjA != null) {
              it.field1 = updatedEntity.field1
          }
          // …
      }
      .onEach { repo.save(it) }
      .map { it.toDto() }
}

Since mutability is used, this version changes the order of operations. You may want to insert asSequence or asFlow at the start of the pipeline to restore the previous order of operations.

Another thing that I find strange here, is that it searches by ID but the code is written using collection functions, meaning it expects multiple elements to be returned. I think this would need clarifying.

[CLOVIS]

(also, you may be interested in #codereview)

[Larry Garfield]

I’ve set up a call with the dev who wrote it later this week to figure out why the code is what it is. I realized it’s being called from only one place, and it’s a place that I wouldn’t expect to be doing anything remotely like this.

The fun you find when joining a new team…

[CAB]

All these solutions are good for different requirements and style preferences.
In original more complete sample the function “map” is absolutely misused: it converting one collection to another. In this sample it seems you retrieving DB record by unique primary key “id”. Even if the “fingById(id)” returns a collection, you should extract single included element and use “run”, “with”, or “apply” methods. I prefer them as they allows to reference fields of the context object directly (without referencing the object). Then inner function “let” or “also” won’t overshadow anything even without defining a local argument of the lambda.
Something like;

fun updateById(id, updatedDto) {
  val updatedEntity = updatedDto.toEntity()
  return repo.run {             // repo -> this
    findById(id).apply {        // DB instance -> this
      // the object from DB is represented here by “this”
      updatedDto.subObjA?.let { // subObjA -> it and not used
        field1 = updatedEntity.field1
        // ...
      }
    }.save(it) // save updated DB instance
     .toDto()  // convert to DTO to return
  } // returns updated DTO
}

[Larry Garfield]

So the update: This code path will be deleted soon-ish anyway, as it will not be necessary after the next feature. So I won’t bother fixing it, just delete it in a few weeks. 🙂 Thanks everyone for the validation.

[Chris Lee]

The only bug-free code is deleted code