Add usecase repository #2

flipper83 · 2017-10-01T19:45:16Z

This pull request contains the first commit with information come from use cases.
I like update the PR step by steps because I have some code that I like your approval :)

This first commit contains usercases, list representation and resultValues.
Next commit will contains a list test and travis configuration.
After that I will start working on repository code.

pedrovgs · 2017-10-02T07:33:58Z

app/src/main/java/com/karumi/ui/domain/Result.kt

+        fun <R> success(value: R) = Result.Right(value)
+        fun <R> error(error: Error) = Result.Left<R>(error)
+    }
+}


If you implemented this class it's great! I guess we don't need more right now. But I'd rename this class as Either and if it's possible, I'd use this or this implementation so we don't need to re-invent the wheel. It seems the libraries implementations are more complete 😃

I've been using funKTionale and it fits perfectly as it is separated in tons of modules that we can load whenever we want (funktionale-either is what we want here)

I took the Kotlin blog implementation :), I renamed from Either to Result because a lot of people don't know what Either means. But I will import funktionale-either.

What is your opinion about limit the Left type to Error except open to any kind of type?

Either represents the disjunction mathematical concept where the left side of the expression can be an error or not. I'd not force the usage of an error in the left side.

If we are going to use always for symbolizing an error? is not good limit the inheritance?

Most of the times we will use it with errors, but not always. In this project, we don't even need to use errors. If you want to port the original version of this kata you don't even need to use Either or Result.

I'm asking for learn :) beacuse If the first time that I implement this kind of things. I like understand why we do some things. Original kata is not writting in kotlin, I like adjust the code to kotlin for learn and discover different ways to develop. Remember that the swift Kata is not similar to the Java one ;)

Don't read me wrong. I'm just trying to give you my feedback as you requested. If you read the Swift implementation of this kata we don't handle errors. If you have any doubt or any concrete question I did not answer, please let me know.

pedrovgs · 2017-10-02T07:35:00Z

app/src/main/java/com/karumi/ui/domain/model/SuperHero.kt

+package com.karumi.ui.domain.model
+
+data class SuperHero(val name: String,
+    val photo: String,


If I remember well. The Marvel API has the picture field as optional.

the problem is if this is optional picasso crash, what do you prefer? check in ui or check in the respository layer?

If you change the picture to optional you'll have to check if the picture is defined before using it in picasso.

pedrovgs · 2017-10-02T07:37:56Z

app/src/main/java/com/karumi/ui/domain/usecase/GetSuperHeroes.kt

+
+class GetSuperHeroes {
+
+    val mockSuperHero = SuperHero(name = "Super Peter",


In the original implementation, we have a simple repository and the values it returns are private. I've read the repository will be your next PR but we should keep this as private.

yes, this line will be dissappear in the next PR, but yes, this is a bug.

pedrovgs · 2017-10-02T07:39:53Z

app/src/main/java/com/karumi/ui/domain/usecase/GetSuperHeroes.kt

+
+    fun get(): Result<List<SuperHero>> {
+        return Result.success(arrayListOf(mockSuperHero))
+    }


You don't even need { } here. You can use just a = and return your value.

yes, thank you!

pedrovgs · 2017-10-02T07:50:22Z

app/src/main/java/com/karumi/ui/presenter/SuperHeroesPresenter.kt


+    private fun refreshSuperHeroes() {
+        async {


The usage of async/await is so cool and so fancy that I love it ❤️ But I was wondering if we can easily configure an idling resource. I've been reading the documentation and I don't see any reference to this topic. If this is the case, we will need to create our own idling resource. For this kata this approach is ok, but w/o the idling resource I'd not use this library.

For the kata is not mandatory, and I like try wrap the async/await in an object. Let me check one idea that I have :P

For the kata is not mandatory. That's why I don't request changes and just added comments 😃 I've created a new entry in our outreach ideas log for implementing this.

pedrovgs · 2017-10-02T07:52:10Z

app/src/main/java/com/karumi/ui/presenter/SuperHeroesPresenter.kt

+            val result = await { getSuperHeroes.get() }
+            view()?.hideLoading()
+            when (result) {
+                is Right -> view()?.showSuperHeroes(result.value)


You are not implementing one of the two main kata behaviors. The empty case implementation.

because is out of the scope of the PR. I will add in a next iteration.

pedrovgs · 2017-10-02T07:54:11Z

app/src/main/java/com/karumi/ui/domain/usecase/GetSuperHeroes.kt

+import com.karumi.ui.domain.Result
+import com.karumi.ui.domain.model.SuperHero
+
+class GetSuperHeroes {


As this language is less verbose you can create a file named usecases.kt and move the two use cases we will have to this file. You can even name it domain and move your abstract data types and use cases there.

Not for me, one of the good part of the Use cases takes a look and check all operations that we can do over our domain, if we put all together in one file this file can grow to fast and is not a benefict for read it.

pedrovgs · 2017-10-02T07:55:30Z

app/src/main/java/com/karumi/ui/view/MainActivity.kt

+                instance())
+        }
+
+        bind<GetSuperHeroes>() with provider { GetSuperHeroes() }


Do you need to bind a class even when the constructor is empty?

yes this is mandatory :( this is one of the things that I don't like from kodein.

Then I'd not use this dependency injector. If we have to define a dependency every time we need it it's going to be a pain in the ass. Imagine you need to use this use case in two different places and this use case depends on a repository. Based on this library you'll need to write this code twice:

bind<GetSuperHeroes>() with provider { GetSuperHeroes(new SuperHeroesRepository()) }

When the dependency tree is simple, this is not an issue but in the future, this will be a pain in the ass. We could reduce the boilerplate by moving this declaration to a module configured just one.

Yes, I have a general module, commons implementation must to be in general module. I am ok with you that it's not the best solution. I don't know why does not automatic.

pedrovgs · 2017-10-02T07:56:55Z

app/src/main/java/com/karumi/ui/view/adapter/SuperHeroViewHolder.kt

+    }
+
+    private fun renderSuperHeroPhoto(photo: String) {
+        Picasso.with(context).load(photo).fit().centerCrop().into(photoImageView)


Remember to check if the photo is defined. The Marvel API declares this attribute as optional but we are not handling it.

I repeat the same question, if we manage the nullability here or in the api. And remember that the kata don't use internet the data comes from hardcoded info in the repositories.

Choose the one you prefer. By definition the picture is optional. If I were you, I'd declare the photo as an optional and I'd check the definition of the photo value in this method, in the UI layer.

I'm trying to know if it's a best option resolve the nullabilities as soon as we found, in this case the api layer or manage in the ui layer. :) I trying to learn because you have more experience with this kind of things.

pedrovgs · 2017-10-02T07:58:06Z

build.gradle

@@ -7,7 +7,7 @@ buildscript {
        jcenter()
    }
    dependencies {
-        classpath 'com.android.tools.build:gradle:3.0.0-beta4'
+        classpath 'com.android.tools.build:gradle:3.0.0-beta6'


Do we need to use the beta version?

yes, do yo ask the same in the previous PR.

I don't remember why it's needed, but don't worry. I'll review the previous PR.

pedrovgs · 2017-10-02T08:00:02Z

Well done @flipper83 😃 As this project aims to be a reference for testing purposes I'd recommend you to start writing tests ASAP. Part of your design will be directly affected by your tests implementation.

Serchinastico

This is looking awesome, thank you for your work Jorge

Serchinastico · 2017-10-02T08:04:04Z

app/src/main/java/com/karumi/ui/domain/Result.kt

+        fun <R> success(value: R) = Result.Right(value)
+        fun <R> error(error: Error) = Result.Left<R>(error)
+    }
+}


I've been using funKTionale and it fits perfectly as it is separated in tons of modules that we can load whenever we want (funktionale-either is what we want here)

Serchinastico · 2017-10-02T08:14:37Z

app/src/main/java/com/karumi/ui/domain/usecase/GetSuperHeroes.kt

+        isAvenger = true,
+        photo = "https://avatars1.githubusercontent.com/u/4030704")
+
+    fun get(): Result<List<SuperHero>> {


Have you considered implementing the invoke operator so that they look like functions instead of having to implement a get, execute, run method?

of course https://kotlinlang.org/docs/reference/operator-overloading.html I will try it.

…ase.

Jorge Juan Barroso Carmona added 2 commits October 1, 2017 20:52

Add Either monad at result object.

acd625b

Add usecase and draw the response.

a9e6c8c

flipper83 requested review from Serchinastico and pedrovgs October 1, 2017 19:45

pedrovgs reviewed Oct 2, 2017

View reviewed changes

Serchinastico approved these changes Oct 2, 2017

View reviewed changes

Jorge Juan Barroso Carmona and others added 10 commits October 3, 2017 10:39

Changes from PR.

3398383

Add travis configuration file

e2312b9

Add Respository file and move domain classes to correct package.

1c6be06

Add repository information, add show and hide spinner and add empty c…

7df4187

…ase.

Add first test screenshot.

a839a58

Add main activity screenshot tests and rename code relate to it.

b704109

remove unnecessary if else

ef81ad7

Update images for mainActivityTest

9886038

remove unnecessary space.

e6926c1

Remove unused line in travis configuration

8dd35ec

Serchinastico merged commit ec6bd10 into master Oct 17, 2017

Serchinastico deleted the add-usecase-repository branch October 17, 2017 16:25


		class GetSuperHeroes {

		val mockSuperHero = SuperHero(name = "Super Peter",

Add usecase repository #2

Add usecase repository #2

Conversation

flipper83 commented Oct 1, 2017

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

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

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

pedrovgs commented Oct 2, 2017

Serchinastico left a 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