[Code health] Consistent naming of local DB converters

[scolsen]

Pretty much every local db entity has to/from conversion methods for converting a corresponding model object to the local db entity type. However, we don't name these consistently. Sometimes they are named toModel, sometimes toObject sometimes to<ModelTypeName>...

We should pick a consistent naming scheme. For extra clarity, my vote goes toward to<ModelTypeNameModel>Model; e.g.

toLocationOfInterestModel; fromLocationOfInterestModel; toAuditInfoModel; fromAuditInfoModel

[scolsen]

@gino-m, @JSunde, @shobhitagarwal1612 -- any opinions on this?

[JSunde]

I think to sounds good 👍

On Thu, Sep 15, 2022 at 10:18 AM Scott Olsen @.***> wrote:

@gino-m https://github.com/gino-m, @JSunde https://github.com/JSunde, @shobhitagarwal1612 https://github.com/shobhitagarwal1612 -- any opinions on this?

— Reply to this email directly, view it on GitHub https://github.com/google/ground-android/issues/1276#issuecomment-1248167136, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACFDANEXWJSA24DHDE4MU3LV6MVZ5ANCNFSM6AAAAAAQNOF5UQ . You are receiving this because you were mentioned.Message ID: @.***>

[shobhitagarwal1612]

toAuditInfoModel makes it seem like model is part of the class name. I'm also slightly leaning towards to<ModelTypeName>

[scolsen]

@gino-m mentioned in our standup that we would ideally decouple these routines from the model/entity classes since it's not clear which should own them anyway. what do we think of this approach:

• new package ground.serialization

• new interface:

  sealed interface Serializer<T, R> {
  fun serialize(model: T): R?
  fun deserialize(record: R): T?
  }

• example impl:

  object GeometrySerializer : Serializer<Geometry, GeometryEntity> {
  override fun serialize(model: Geometry): GeometryEntity {
  TODO("Not yet implemented")
  }
  
  override fun deserialize(record: GeometryEntity): Geometry? {
  when(record.geometryType) {
    GeometryType.POINT.name -> null
    GeometryType.POLYGON.name -> null
    else -> null
  }
  //.... other stuff we need to convert to/from these classes
  }

[scolsen]

then calling a conversion to/from a model class would look like: GeometrySerializer.serialize(geom) GeometrySerializer.deserialize(geomentity)

[gino-m]

+1, modulo "serialize", which implies it's actually converting to a stream of bytes that can be persisted. I think Converter is more correct, since we're converting between object types.

Also +1 to to<layername>... so for full clarity we'd have something like GeometryConverter.toLocalDbEntity and GeometryConverter.fromLocalDbEntity. I believe GeometryConverter would only be used by the LocaDbDatastore, so it can be marked internal, and shouldn't conflict with other GeometryConverters.

Thoughts?

[scolsen]

+1, modulo "serialize", which implies it's actually converting to a stream of bytes that can be persisted. I think Converter is more correct, since we're converting between object types.

Also +1 to to<layername>... so for full clarity we'd have something like GeometryConverter.toLocalDbEntity and GeometryConverter.fromLocalDbEntity. I believe GeometryConverter would only be used by the LocaDbDatastore, so it can be marked internal, and shouldn't conflict with other GeometryConverters.

Thoughts?

I agree that Converter would be better, but I worry about it because we already have some many other classes called Converter. I've been trying to find an alternative word; I had the same reservations about serialize but settled on it because wikipedia's definition was general enough to encompass our case: https://en.wikipedia.org/wiki/Serialization (converting data into a storable format)...but I also agree that it suggests raw binary data.

[gino-m]

I agree that Converter would be better, but I worry about it because we already have some many other classes called Converter

If they are all modeling the same pattern (converting from the representation in one layer of abstraction to another), that's a good thing! Having two names for the same pattern could actually be detrimental since it would imply they're somehow doing something fundamentally different.

In my experience the most confusing part of our converter method names is "what is being converted to what". If we assume converters all either convert to or from our domain model, the only part that changes is what other layer they're being converted to/from. That's why I lated on FooConverter.(to|from)LocalDbEntity. Still open to ideas, just wanted to get down my logic for the above suggestion for posterity.

[scolsen]

I agree that Converter would be better, but I worry about it because we already have some many other classes called Converter

If they are all modeling the same pattern (converting from the representation in one layer of abstraction to another), that's a good thing! Having two names for the same pattern could actually be detrimental since it would imply they're somehow doing something fundamentally different.

In my experience the most confusing part of our converter method names is "what is being converted to what". If we assume converters all either convert to or from our domain model, the only part that changes is what other layer they're being converted to/from. That's why I lated on FooConverter.(to|from)LocalDbEntity. Still open to ideas, just wanted to get down my logic for the above suggestion for posterity.

I think we're approaching the right solution; what about:

internal sealed interface Converter<T,R> 

interface LocalDBConverter<T,R> : Converter<T,R> {
  fun toLocalDBEntity(...) {... }
  fun fromLocalDBEntity(...)  {...}
}

And we'd have similar interfaces for other layers.

[gino-m]

I think that makes sense. I haven't made up my mind about using inheritance solely for the purpose of enforcing naming conventions (ie LocalDbConverter would never be referenced except by implementors), but the pattern make sense.

Nit: Following Java naming conventions (assuming they're the same in Kotlin) here we'd have LocalDb and not LocalDB.

Thanks for working through this!

[scolsen]

Sorry to renege on this, but I think we might be better off using extension functions instead of conversion objects.

It turns out we don't really have an interface as many of the conversions require accompanying data--that is, the object being converted often isn't the only thing we need to perform the conversion. For example, conversions from MultipleChoiceEntity to MultipleChoice require us to pass along a set of options. We can do this w/ classes, but it becomes awkward as we often only need accompanying data for one side of the conversion, so we either need to encapsulate certain things in companions and use dispatch or we need to pass constructor arguments to classes. In other words, we don't really have a unified interface for conversions and using one is needlessly complex.

Instead, I think we can use extension functions following the convention that the conversion function is always defined on the object we're converting from, e.g.:

fun MultipleChoiceEntity.toMultipleChoice(optionEntities: List<OptionEntity>): MultipleChoice {
  val listBuilder = ImmutableList.builder<Option>()

  for (optionEntity in optionEntities) {
    listBuilder.add(OptionEntity.toOption(optionEntity))
  }

  return MultipleChoice(listBuilder.build().toPersistentList(), this.type.toCardinality())
}

fun MultipleChoice.toMultipleChoiceEntity(taskId: String): MultipleChoiceEntity =
  MultipleChoiceEntity(taskId, MultipleChoiceEntityType.fromCardinality(this.cardinality))

At call sites this allows us to write:

multipleChoice = multipleChoiceEntity.toMultipleChoice(taskEntityAndRelations.optionEntities)
// instead of: 
// multipleChoice = MultipleChoiceConverter(nullTaskId, taskEntityAndRelations.optionEntities).toMultipleChoice(multipleChoiceEntity)

multipleChoiceDao
      .insertOrUpdate(multipleChoice.toMultipleChoiceEntity(taskId))
      .andThen(insertOrUpdateOptions(taskId, multipleChoice.options))
      .subscribeOn(schedulers.io())
 // instead of:
// multipleChoiceDao
      // we don't even use the options for this one!
     // .insertOrUpdate(MultipleChoiceConverter(taskId, emptyOptions).toMultipleChoiceEntity(multipleChoice))
     // .andThen(insertOrUpdateOptions(taskId, multipleChoice.options))
     // .subscribeOn(schedulers.io())

I think this ends up being a lot clearer and is more flexible given different conversions have different needs.

I propose we stick to implementing the conversion methods on the object that's being converted, so that it's always this in the context of the method body and that we call these methods to<DestinationTypeName>.

While using extensions allows us to decouple the conversions from the declaration of the data classes themselves, there's still something of a downside here in that we are making a choice to bind these methods to particular classes along the to/from boundaries, but overall the resulting code is a lot cleaner imo.

[gino-m]

Good point about not having a standard interface. We may have fallen for the classic pattern-is-an-interface fallacy (not sure if that's a thing, but probably should be).

Are there any downsides to using extension functions to define a method that will only be invoked in one place, rather than for a common behavior that is meant to be shared broadly? It doesn't seem so; there seems to be little/no additional boilerplate to declare these functions as extension functions rather than bare functions, and the benefit is slight better readability (e.g., multipleChoiceEntity.toModelObject(arg1, arg2) vs toModelObject(multipleChoiceEntity, arg1, arg2)?

Also, how do we decide whether the methods are cast as toModelObject() vs fromLocalDbEntity()? Would it be clearer if all the extension functions were on model objects, since they're the common denominator among conversions?

Side note: Imo, including the name of the abstraction layer in conversion methods (i.e., to/from "model object", "local db entity") rather only including the layer-specific language (to/from "multiple choice", "multiple choice entity") is clearer as doesn't require the reader to know that unqualified object names belong to the domain model, and that those qualified with "entity" refer to local db objects.

[scolsen]

Good point about not having a standard interface. We may have fallen for the classic pattern-is-an-interface fallacy (not sure if that's a thing, but probably should be).

Are there any downsides to using extension functions to define a method that will only be invoked in one place, rather than for a common behavior that is meant to be shared broadly? It doesn't seem so; there seems to be little/no additional boilerplate to declare these functions as extension functions rather than bare functions, and the benefit is slight better readability (e.g., multipleChoiceEntity.toModelObject(arg1, arg2) vs toModelObject(multipleChoiceEntity, arg1, arg2)?

Also, how do we decide whether the methods are cast as toModelObject() vs fromLocalDbEntity()? Would it be clearer if all the extension functions were on model objects, since they're the common denominator among conversions?

Side note: Imo, including the name of the abstraction layer in conversion methods (i.e., to/from "model object", "local db entity") rather only including the layer-specific language (to/from "multiple choice", "multiple choice entity") is clearer as doesn't require the reader to know that unqualified object names belong to the domain model, and that those qualified with "entity" refer to local db objects.

Yep, that's definitely a valid approach too. After writing a couple extension functions they seem to be a much better fit for this case imo and the code is a lot more readable, I don't see any apparent downsides beyond the fact that we wind up with a file of a bunch of extension functions, but that seems perfectly fine to me and it allows us to drop/add conversions without having to touch model/entity objects themselves. We can make the extensions have whatever visibilities we need too! re: naming, what about:

model.toLocalDataStoreObject(...)
model.fromLocalDataStoreObject(...)

[scolsen]

Oh, one downside to putting everything on model objects is that the usage is less ergonomic when we're going from an entity back to a model. Instead of writing:

someModelObject.toEntityObject(associatedData)
someEntityObject.toModelObject(associatedData)

we'd have to write:

someModelObject.toEntityObject(associatedData)
ModelClass.fromEntityObject(entity, associatedData)

[scolsen]

We could land somewhere in the middle:

model.toLocalDataStoreObject()
entity.toModelObject()

[gino-m]

The last proposal sgtm, all things considered.

On Fri, Sep 23, 2022, 11:35 AM Scott Olsen @.***> wrote:

We could land somewhere in the middle:

model.toLocalDataStoreObject() entity.toModelObject()

— Reply to this email directly, view it on GitHub https://github.com/google/ground-android/issues/1276#issuecomment-1256368221, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXVUQ7AC6TR6JXJXB5VPTV7XE4JANCNFSM6AAAAAAQNOF5UQ . You are receiving this because you were mentioned.Message ID: @.***>