Domain logic bug in the models #73

ntnsmirnov · 2020-12-29T13:41:17Z

#60
For now:
core.api.domain.model.ICharacter - core character entity
detail.api.domain.model.ICharacterDetail - extension for ICharacter
favorite.impl.data.model.CharacterFavorite - DB entitiy

michaem · 2021-01-06T13:09:08Z

...go/blockme/feature/characters/favorite/api/domain/usecase/IDeleteCharacterFavoriteUseCase.kt

@@ -1,8 +1,9 @@
 package com.stepango.blockme.feature.characters.favorite.api.domain.usecase

-import com.stepango.blockme.feature.characters.favorite.api.domain.model.ICharacterFavorite
+import com.stepango.blockme.feature.characters.core.api.domain.model.ICharacter
+


redundant space

b1n0m13 · 2021-01-06T13:18:10Z

...n/java/com/stepango/blockme/feature/characters/favorite/impl/data/model/CharacterFavorite.kt


 @Entity(tableName = "character_favorite")
 data class CharacterFavorite(
    @PrimaryKey override val id: Long,
    @ColumnInfo(name = "name") override val name: String,
+    @ColumnInfo(name = "description") override val description: String,


Did you forgot to add this new field within MIGRATION_1_2?

I just drop table in MIGRATION_1_2

michaem · 2021-01-06T13:20:17Z

...pi/src/main/java/com/stepango/blockme/feature/characters/core/api/domain/model/ICharacter.kt


-interface ICharacterItem {
+interface ICharacter {


Good improvement!
Now we have only 3 types of "Character"!
But we can do more, what do you think, if we'll remove CharacterItem and CharacterDetail? I think about added single entity implementation of ICharacter into core:impl, for example
class Character : ICharacter
And other domains Character Details/Items/Favorites will use only ICharacter type with default core implementation under hood.

In that case, will be only one Character single entity with unique structure placed in the core target.
What do you think?

b1n0m13 · 2021-01-06T13:20:35Z

...kme/feature/characters/favorite/impl/data/database/migrations/MarvelDatabaseMigration1to2.kt

@@ -21,6 +21,6 @@ import androidx.sqlite.db.SupportSQLiteDatabase

 val MIGRATION_1_2: Migration = object : Migration(1, 2) {


Lack of tests

May be need another issue for that

michaem · 2021-01-06T13:24:54Z

...ain/java/com/stepango/blockme/feature/characters/detail/api/domain/model/ICharacterDetail.kt

+import com.stepango.blockme.feature.characters.core.api.domain.model.ICharacter
+
+interface ICharacterDetail : ICharacter


For details we can use ICharacter type too, cause it doesn't have unique entity logic.
(it will be not scalable if details will be extended, but for our sample it doesn't need)

michaem · 2021-01-06T13:27:12Z

...pango/blockme/feature/characters/favorite/api/domain/usecase/IGetCharacterFavoriteUseCase.kt


 interface IGetCharacterFavoriteUseCase {

-    suspend operator fun invoke(characterFavoriteId: Long): ICharacterFavorite?
+    suspend operator fun invoke(characterFavoriteId: Long): ICharacter?


I think usecase shouldn't throw nullable behavior for "clients", but for improving that need another issue)

michaem · 2021-01-06T13:27:51Z

...kme/feature/characters/favorite/impl/data/database/migrations/MarvelDatabaseMigration1to2.kt

@@ -21,6 +21,6 @@ import androidx.sqlite.db.SupportSQLiteDatabase

 val MIGRATION_1_2: Migration = object : Migration(1, 2) {


May be need another issue for that

michaem · 2021-01-06T13:31:39Z

...om/stepango/blockme/feature/characters/list/impl/data/datasource/CharactersPageDataSource.kt

@@ -41,14 +41,14 @@ const val PAGE_MAX_ELEMENTS = 50
 open class CharacterPageDataSource @Inject constructor(
    private val repository: MarvelRepository,


I think MarvelRepository should use ICharacter type too.
And CharacterMapper we can move into core part.

ntnsmirnov · 2021-01-16T13:39:42Z

...c/main/java/com/stepango/blockme/feature/characters/core/api/data/mapper/ICharacterMapper.kt

+import com.stepango.blockme.feature.characters.core.api.data.response.CharacterResponse
+import com.stepango.blockme.feature.characters.core.api.domain.model.ICharacter
+
+interface ICharacterMapper {


I can't use Mapper<> from :common.utils in this api module

What is the purpose of exposing the mapper in API module?

You shouldn't use CharacterMapper as additional interface for api target. This logic need to place in the impl target as "inner" behavior for converting models.

In the api targets better provide for outer dependecies:

interfaces for domain models

interfaces for UseCase's

interfaces for Repositories

michaem · 2021-01-23T10:44:11Z

...com/stepango/blockme/feature/characters/detail/impl/presentation/CharacterDetailViewModel.kt

@@ -48,7 +48,7 @@ class CharacterDetailViewModel @Inject constructor(
        viewModelScope.launch {
            try {
                val result = marvelRepository.getCharacter(characterId)


You should use call UseCase here, instead directly call to data layer

…aracter

ntnsmirnov requested a review from michaem December 29, 2020 13:41

ntnsmirnov linked an issue Dec 29, 2020 that may be closed by this pull request

Domain logic bug in the models #60

Closed

michaem reviewed Jan 6, 2021

View reviewed changes

b1n0m13 reviewed Jan 6, 2021

View reviewed changes

michaem reviewed Jan 6, 2021

View reviewed changes

b1n0m13 reviewed Jan 6, 2021

View reviewed changes

michaem reviewed Jan 6, 2021

View reviewed changes

ntnsmirnov force-pushed the characters_hell branch from 5f1ccac to 05cf3bd Compare January 16, 2021 13:37

ntnsmirnov commented Jan 16, 2021

View reviewed changes

ntnsmirnov requested a review from michaem January 21, 2021 14:34

michaem reviewed Jan 23, 2021

View reviewed changes

ntnsmirnov added 6 commits January 23, 2021 17:24

#60 list.api.domain.model.ICharacterItem -> core.api.domain.model.ICh…

928eff7

…aracter

#60 CharacterDetail now inherits from ICharacter

eafebf7

#60 ICharacterFavorite deleted. It changed to ICharacter.

d75ce2c

#60 Character entity and CharacterMapper

d064be3

#60 CharacterDetail removed

298b837

#60 CharacterItem removed

7422ea0

ntnsmirnov force-pushed the characters_hell branch from 05cf3bd to 79babb4 Compare January 23, 2021 14:59

ntnsmirnov requested a review from michaem January 23, 2021 15:03

#60 Mapper moved to repository

d327c8a

ntnsmirnov force-pushed the characters_hell branch from 79babb4 to d327c8a Compare January 23, 2021 15:31

michaem approved these changes Jan 24, 2021

View reviewed changes

ntnsmirnov merged commit 5ae58ce into master Jan 25, 2021

ntnsmirnov deleted the characters_hell branch January 25, 2021 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain logic bug in the models #73

Domain logic bug in the models #73

ntnsmirnov commented Dec 29, 2020 •

edited

michaem Jan 6, 2021

b1n0m13 Jan 6, 2021 •

edited

ntnsmirnov Jan 16, 2021

michaem Jan 6, 2021

b1n0m13 Jan 6, 2021

michaem Jan 6, 2021

michaem Jan 6, 2021

michaem Jan 6, 2021

michaem Jan 6, 2021

michaem Jan 6, 2021

ntnsmirnov Jan 16, 2021

stepango Jan 20, 2021

michaem Jan 23, 2021

michaem Jan 23, 2021

		@@ -21,6 +21,6 @@ import androidx.sqlite.db.SupportSQLiteDatabase

		val MIGRATION_1_2: Migration = object : Migration(1, 2) {

		import com.stepango.blockme.feature.characters.core.api.domain.model.ICharacter

		interface ICharacterDetail : ICharacter

		@@ -41,14 +41,14 @@ const val PAGE_MAX_ELEMENTS = 50
		open class CharacterPageDataSource @Inject constructor(
		private val repository: MarvelRepository,

Domain logic bug in the models #73

Domain logic bug in the models #73

Conversation

ntnsmirnov commented Dec 29, 2020 • edited

Choose a reason for hiding this comment

b1n0m13 Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntnsmirnov commented Dec 29, 2020 •

edited

b1n0m13 Jan 6, 2021 •

edited