Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Frist Delivery #43

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juangrind
Copy link

-Setup
-Navigation
-Dependency Injection
-Retrofit adapter and dependency injection.
-MVVM
-Add visual items

Juan Guzman added 2 commits January 31, 2023 17:03
-Navigation
-Dependency Injection
-Retrofit adapter and dependency injection.
-MVVM
-Add visual items
Copy link

@mrarrieta mrarrieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buen trabajo, deje unos comentarios

android:padding="16dp"
android:orientation="vertical">

<ImageView

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ten cuidado con la indentacion

app:layout_constraintTop_toTopOf="parent"/>

<TextView
android:id="@+id/txt_MinimumPrice"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usa cammel case o snake case en los nombres de los ids pero debes ser consistente.

import android.net.ConnectivityManager

object BaseUtils {
var context: Context? = null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

para que conservas el contexrto aca?

): View? {
_binding = FragmentCryptoListBinding.inflate(inflater, container, false)
val view = binding.root
return view }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ten cuidado con la indentacion

}

cryptoViewModel.onCreateTicker(bookName)
cryptoViewModel.tickerState.observe(viewLifecycleOwner) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Puedes tener 1 solo Live data si emites estados distintos con la informacion necesaria

){
suspend operator fun invoke(): List<BooksModelDomain> {
val books = cryptoRepository.getAllAvailableBooksFromApi()
return if (books.isNotEmpty()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el use case debe capturar las excepciones

suspend operator fun invoke(book: String): List<AsksModelDomain> {
val asks = cryptoRepository.getAllAsksFromApi(book)

return if (asks.isNotEmpty()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el use case debe capturar las excepciones

import kotlinx.coroutines.withContext
import javax.inject.Inject

class CryptoService @Inject constructor(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creo que CryptoRemoteDataSource es mas claro como nombre

import com.jpgl.cryptocurrencies.domain.model.*
import javax.inject.Inject

class CryptoRepository @Inject constructor(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el use case no deberia saber si esta consumiendo la informacion desde un api o desde una base de datos local, se supone que el repo abstrae todo esto, no lo expone.

import retrofit2.converter.gson.GsonConverterFactory

object RetrofitConfig {
fun getConfigRetrofit(): Retrofit {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creo que esto ya no se usa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants