-
Notifications
You must be signed in to change notification settings - Fork 55
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
0/primer entregable #46
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hola Omar buen día. Como te comente en la sesión anterior, tu primer entregable me pareció muy bueno, integraste correctamente el API de Bitso, implementaste la arquitectura MVVC de manera adecuada, incluyendo una capa extra como son los UseCases. Aunque no era necesario, también agregaste adecuadamente la parte de autenticación para el API. Te agrego unos comentarios en tu código para mejorarlo, en especial podrías usar ListAdapter para optimizar la lista de monedas y encapsular tus liveData que usas en tu ViewModel. También podrías mejorar tu pantalla de detalles ya que por ahora solo se muestra en texto la respuesta de los 2 servicios.
import android.content.res.Resources | ||
|
||
class ZeroCoinsApplication: Application() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remueve todos estas lineas en blanco para mejor legibilidad de tu código
@@ -0,0 +1,3 @@ | |||
package com.axiasoft.android.zerocoins.common | |||
|
|||
fun String.emptyString(): String = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esta función no la usas en tu proyecto. Si no la necesitas considera removerla de tu código.
xrp_mxn(coinName = "xrp_mxn", coinImage = R.drawable.ic_xrp, coinKey = "xrp_mxn"), | ||
ltc_btc(coinName = "ltc_btc", coinImage = R.drawable.ic_litecoin, coinKey = "ltc_btc"), | ||
ltc_mxn(coinName = "ltc_mxn", coinImage = R.drawable.ic_litecoin, coinKey = "ltc_mxn"), | ||
bch_btc(coinName = "btc_mxn", coinImage = R.drawable.ic_bch, coinKey = "bch_btc"), | ||
bch_mxn(coinName = "bch_mxn", coinImage = R.drawable.ic_bch, coinKey = "btc_mxn"), | ||
tusd_btc(coinName = "tusd_btc", coinImage = R.drawable.ic_btc, coinKey = "tusd_btc"), | ||
mana_btc(coinName = "mana_btc", coinImage = R.drawable.ic_btc, coinKey = "mana_btc"), | ||
|
||
mana_mxn(coinName = "mana_mxn", coinImage = R.drawable.ic_cryptocurrency, coinKey = "mana_mxn"), | ||
bat_btc(coinName = "bat_btc", coinImage = R.drawable.ic_btc, coinKey = "bat_btc"), | ||
btc_usd(coinName = "btc_usd", coinImage = R.drawable.ic_btc, coinKey = "btc_usd"), | ||
xrp_usd(coinName = "xrp_usd", coinImage = R.drawable.ic_xrp, coinKey = "xrp_usd"), | ||
|
||
btc_dai(coinName = "btc_dai", coinImage = R.drawable.ic_btc, coinKey = "btc_dai"), | ||
dai_mxn(coinName = "dai_mxn", coinImage = R.drawable.ic_cryptocurrency, coinKey = "dai_mxn"), | ||
bat_mxn(coinName = "bat_mxn", coinImage = R.drawable.ic_cryptocurrency, coinKey = "bat_mxn"), | ||
btc_ars(coinName = "btc_ars", coinImage = R.drawable.ic_btc, coinKey = "btc_ars"), | ||
|
||
|
||
eth_usd(coinName = "eth_usd", coinImage = R.drawable.ic_ethereum, coinKey = "eth_usd"), | ||
dai_ars(coinName = "dai_ars", coinImage = R.drawable.ic_btc, coinKey = "dai_ars"), | ||
btc_brl(coinName = "btc_brl", coinImage = R.drawable.ic_btc, coinKey = "btc_brl"), | ||
eth_ars(coinName = "eth_ars", coinImage = R.drawable.ic_ethereum, coinKey = "eth_ars"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Falta agregar un nombre legible para todas estas monedas
|
||
data class Book ( | ||
|
||
@SerializedName("book" ) var book : String? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remueve espacios en blanco que no sean necesarios
@SerializedName("book" ) var book : String? = null, | ||
@SerializedName("price" ) var price : String? = null, | ||
@SerializedName("amount") var amount : String? = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remueve espacios en blanco que no sean necesarios
log(message = "Hi array of books:-> ${response.response.payload.toString()}") | ||
} | ||
} | ||
else -> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Falto agregar manejo de error para este caso
tickerState.value = TickerScreenState.TickerSuccess(ticker) | ||
} | ||
is TickerScreenState.TickerError -> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Falto agregar manejo de error para este caso
listOrderBookScreenState.value = listOrderBookState | ||
} | ||
is ListOrderBookScreenState.ErrorOrEmpty -> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Falto agregar manejo de error para este caso
//viewModel.getBooks() | ||
/*viewModel.getBooksWithUseCase() | ||
viewModel.books.observeForever { | ||
binding.tvTest.text = it.toString() | ||
}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si este código no lo necesitas considera removerlo
import com.axiasoft.android.zerocoins.features.coins.domain.models.data.book.response.Book | ||
import com.bumptech.glide.Glide | ||
|
||
class BookOrderAdapter(private val onItemClick: (Book) -> Unit): RecyclerView.Adapter<BookOrderAdapter.BookOrderViewHolder>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Podrias optimizar tu código usando ListAdapter en lugar de RecyclerView.Adapter.
Primer entregable
Pantalla mock detalle orderbook del servicio con texto de objetos de lista de bids y asks haciendo uso del servicio order-book y ticker
Projecto base con arquitectura MVVM y LiveData con cliente retrofit y GSON adapter
Pantalla mock de lista de las monedas utilizando el servicio available-books