-
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
Primer entregable #28
base: main
Are you sure you want to change the base?
Conversation
-Pantalla de lista de monedas -Pantalla de detalle
class AvailableBooksAdapter( | ||
private var data: List<AvailableBook> = emptyList<AvailableBook>(), | ||
private val goToDetail: (AvailableBook?) -> Unit, | ||
) : RecyclerView.Adapter<AvailableBooksAdapter.ViewHolder>() { |
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.
Evita usar directamente RecyclerView.Adapter, ya que es poco eficiente. Puedes usar ListAdapter, el cual se apoya en los DiffCallbacks para mejorar la eficiencia de listas.
|
||
inner class ViewHolder(private val itemBinding: ItemOpenOrderBinding) : RecyclerView.ViewHolder(itemBinding.root) { | ||
fun bindItem(item: OpenOrder) = with(itemBinding) { | ||
openOrderPrice.text = item.price.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.
Para que tu interfaz se vea más llamativa al usuario, podrías crear una función de extensión que transforme un número a un formato de moneda.
.addConverterFactory(GsonConverterFactory.create(gson)) | ||
.build() | ||
|
||
fun repository(): BitsoApi = getRetrofit().create(BitsoApi::class.java) |
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.
Por favor cambia el nombre de este método. Un API/service, no es un repository.
@SerializedName("payload") | ||
@Expose | ||
var payload: List<AvailableBookDto>? = null | ||
):Serializable |
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.
En este caso, no es necesario que el modelo implemente Serializable
(*Aplica también para TickerResponse y TickerDto)
bids = bids.toOrderBookList | ||
) | ||
|
||
private val List<OpenOrderResponse>?.toOrderBookList: List<OpenOrder> |
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.
Revisa por favor las funciones de extensión, la nomenclatura es un poco distinta:
https://kotlinlang.org/docs/extensions.html#extensions-are-resolved-statically
|
||
binding.apply { | ||
criptoCurrencyVM.isLoading.observe(viewLifecycleOwner) { | ||
rvBooks.adapter = AvailableBooksAdapter(criptoCurrencyVM.availableOrderBookList.value ?: emptyList(), |
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.
Un Observer se llama con cada cambio del Observable, por esta razón crear un adapter dentro de un Observer puede ser costoso para UI y puede generar memory leaks.
Evita instanciar adapters en Observers, en cambio, crea el adapter por fuera y usa un método para actualizar la lista de datos (por ejemplo, para un ListAdapter puedes usar submitList)
import kotlinx.coroutines.flow.onEach | ||
import javax.inject.Inject | ||
|
||
@HiltViewModel |
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.
Al igual que @Inject, @HiltViewModel es una anotación característica del framework de inyección de dependencias. Aún no tienes implementado Hilt por completo en tu aplicación, así que en este momento, esta anotación no es necesaria.
private val orderBookUseCase: OrderBookUseCase | ||
) : ViewModel() { | ||
|
||
private var _availableOrderBookList = MutableLiveData<List<AvailableBook>>() |
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.
Estas variables deberían ser tipo val.
_isLoading.value = false | ||
} | ||
is RequestState.Error -> { | ||
error(it.message ?: "") |
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.
Similar al comentario en el Fragment, el manejo de errores debe hacerse mediante observables en lugar de callbacks desde la UI.
import javax.inject.Inject | ||
|
||
@HiltViewModel | ||
class CriptoCurrencyViewModel @Inject constructor( |
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.
Por convención, para cada Fragmento hay un ViewModel respectivo que se encarga de su estado.
Por favor, agrega AvailableBooksViewModel y OrderBookDetailViewModel
-Pantalla de lista de monedas
-Pantalla de detalle