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

Segundo entregable #32

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

Conversation

AldoCrono
Copy link

-Entities, Dao and local data source (in progress)
-Added Room Module, logic in AvailableBooksFragment
-Initialize localDataSource on AvailableBooksFragment
-Create InitApplication class to build database
-Added OkHttp Library
-HttpLoggingInterceptor
-Added User-Agent Header
-Added Extention Functions to call toast with less code

Copy link

@diego-parra-robayo diego-parra-robayo left a comment

Choose a reason for hiding this comment

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

Buenos días Aldo. Te dejo algunos comentarios respecto a tu segundo entregable.
Trabajaste en la mayoría de puntos requeridos, pero te hizo falta agregar algunas pruebas unitarias. Por favor, agregalas antes de tu tercer entregable.

class AvailableBooksAdapter(
private var data: List<AvailableBook> = emptyList(),
private val goToDetail: (availableBook:AvailableBook?,coinName: String) -> Unit,
) : RecyclerView.Adapter<AvailableBooksAdapter.ViewHolder>() {

Choose a reason for hiding this comment

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

Por favor, actualiza este Adapter a ListAdapter u otra implementación con DiffCallbacks.
ListAdapter es una implementación ya con algunos años desde su lanzamiento y tiene grandes ventajas en cuanto a la eficiencia de las listas.


class OpenOrdersAdapter(
private var data: List<OpenOrder> = emptyList(),
) : RecyclerView.Adapter<OpenOrdersAdapter.ViewHolder>() {

Choose a reason for hiding this comment

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

Igual que con AvailableBooksAdapter, actualiza también este adapter a ListAdapter

itemBinding.apply {
val coinName=getCoinName(item.book.toString())
tvCoinName.text = coinName
tvValueMin.text= "Mínimo :"+item.minimum_value.toString()

Choose a reason for hiding this comment

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

Como buena práctica, es mejor colocar strings en los archivos de resources.

tvValueMax.text= "Máximo :"+item.maximum_value.toString()
imgCoin.setImageDrawable(ContextCompat.getDrawable(context.applicationContext, getCoinImage(item.book.orEmpty())))

itemView.setOnClickListener {

Choose a reason for hiding this comment

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

Este es un detalle menor, pero bindItem es un método que se llama constantemente. Para mejorar un poco la eficiencia, este setOnClickListener podría crearse en el constructor (init) del ViewHolder en lugar de dentro de bindItem.


private val okHttpClient: OkHttpClient =
OkHttpClient.Builder()
.addInterceptor { chain ->

Choose a reason for hiding this comment

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

Este código puede mejorar y ser más limpio. Logging interceptor y User-Agent interceptor son distintos y se pueden agregar al OkHttpClient.Builder en diferentes métodos de addInterceptor { ... }.
Así tampoco se requeriría agregar otro OkHttpClient.Builder dentro de addInterceptor.

savedInstanceState: Bundle?
): View {
binding = FragmentAvailableBooksBinding.inflate(layoutInflater, container, false)
criptoCurrencyDB= Room.databaseBuilder(

Choose a reason for hiding this comment

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

Crear una base de datos es computacionalmente costoso. No deberías crear la base de datos en este punto, ya que onCreateView es un método que se repite muy frecuentemente.
En este caso, deberías usar la base de datos que creaste en la clase Application a la cual puedes acceder a través del context, o acceder a la dependencia mediante Hilt (cuando tengas la implementación completa).

import androidx.room.Room
import com.wizeline.criptocurrency.data.database.CriptoCurrencyDB

class InitApplication:Application() {

Choose a reason for hiding this comment

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

Si decides usar esta clase como tu aplicación, debes registrarla en el Manifest.

import javax.inject.Inject

@HiltViewModel
class CriptoCurrencyViewModel @Inject constructor(

Choose a reason for hiding this comment

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

Como te mencioné en los comentarios del primer entregable, deberías tener un ViewModel por cada Fragment.

//Separar ViewModel para cada fragment

fun getAvailableBooksCoroutine() {
CoroutineScope(Dispatchers.Main).launch{

Choose a reason for hiding this comment

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

CoroutineScope te crea un nuevo scope para las coroutinas. Si decides hacerlo manualmente, debes guardar una referencia al nuevo scope, y limpiarlo en el método onCleared del viewModel.
La opción más sencilla es usar viewModelScope, específico para los ViewModels y que ya incluye la limpieza en onCleared. Simplemente puedes reemplazarlo por viewModelScope.launch { ... }

app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintBottom_toTopOf="@id/ll_bids_asks"/>

<LinearLayout

Choose a reason for hiding this comment

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

El propósito de ConstraintLayout es evitar demasiadas vistas internas ya que afectan la eficiencia.
En la medida de lo posible, evita crear layouts internos como este y coloca las vistas necesarias directamente sobre el constraintLayout. Para ubicarlas usa los attributos propios de ConstraintLayout como layout_constraintStart_toStartOf, layout_constraintEnd_toEndOf, ...

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