0/tercer entregable #50
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and db models for exchange order books + poc db room + room dependencies + refactor book to exchange boor order + poc dao
… + update primary key for asks and bids + stores ticker to db + item layout for bids and ask
added 23 commits
February 17, 2023 12:57
- add book name on bids and orders - moved internet checker to activity viewmodel
…fragment + temp cache strategy for available books call request to avoid multiple calls + fixed exposed mutable live data on view model
…ta a arquitectura MVVM con use case
…xchange order books
emm4nuelBar
reviewed
Mar 5, 2023
There was a problem hiding this comment.
Hola Omar,
Te felicito por tu proyecto!! me dio mucho gusto ver tu progreso a lo largo de cada semana. Para este entregable te dejo unos comentarios sobre DI y buenas practicas. Son detalles menores que podrían mejorar tu código, en general es un muy buen trabajo!!
Comment on lines
+11
to
+58
|
|
||
| abstract class HttpConnectionManager<T : Any>(coinApi: CoinApis) { | ||
|
|
||
| abstract fun build(): T | ||
|
|
||
| private val loggingInterceptor = HttpLoggingInterceptor().apply { | ||
| level = if (BuildConfig.DEBUG) { | ||
| HttpLoggingInterceptor.Level.BODY | ||
| } else { | ||
| HttpLoggingInterceptor.Level.NONE | ||
| } | ||
| } | ||
|
|
||
| val httpClient = when (coinApi) { | ||
| CoinApis.BITSO -> { | ||
| Retrofit.Builder() | ||
| .baseUrl(CoinApis.BITSO.hostUrl) | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .addCallAdapterFactory(RxJava2CallAdapterFactory.create()) | ||
| .client( | ||
| OkHttpClient().newBuilder() | ||
| .addInterceptor(BitsoInterceptor()) | ||
| .addInterceptor(loggingInterceptor) | ||
| .build(), | ||
| ).build() | ||
| } | ||
| CoinApis.RESTFUL_API -> { | ||
| Retrofit.Builder() | ||
| .baseUrl(CoinApis.RESTFUL_API.hostUrl) | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .client( | ||
| OkHttpClient().newBuilder() | ||
| .addInterceptor(loggingInterceptor) | ||
| // .addInterceptor(BitsoInterceptor()) | ||
| // .addNetworkInterceptor(GSSCHeadersInterceptorAWSS3()) | ||
| .build(), | ||
| ).build() | ||
| } | ||
|
|
||
| else -> { | ||
| Retrofit.Builder() | ||
| .baseUrl("") | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .client(OkHttpClient().newBuilder().build()) | ||
| .build() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Podrías mejorar este código creando un modulo y usando inyección de dependencias para obtener la instancia de retrofit, esta seria una propuesta:
@Module
@InstallIn(SingletonComponent::class)
object NetworkingRxModule {
private val loggingInterceptor = HttpLoggingInterceptor().apply {
level = if (BuildConfig.DEBUG) {
HttpLoggingInterceptor.Level.BODY
} else {
HttpLoggingInterceptor.Level.NONE
}
}
@Provides
@Singleton
fun provideRetrofitClient(): Retrofit {
return Retrofit.Builder()
.baseUrl(CoinApis.BITSO.hostUrl)
.addConverterFactory(GsonConverterFactory.create())
.addCallAdapterFactory(RxJava2CallAdapterFactory.create())
.client(
OkHttpClient().newBuilder()
.addInterceptor(BitsoInterceptor())
.addInterceptor(loggingInterceptor)
.build(),
).build()
}
}
Comment on lines
+27
to
+30
| @Singleton | ||
| fun provideBitsoOrderBooksApi(): BitsoOrderBooksApi { | ||
| return BitsoOrderBooksApi.Builder().build() | ||
| } |
There was a problem hiding this comment.
Al usar DI para obtener tu instancia de retrofit, este metodo podría refactorizarse a
@Provides
@Singleton
fun provideBitsoOrderBooksApi(retrofit: Retrofit): BitsoOrderBooksApi {
return retrofit.create(BitsoOrderBooksApi::class.java)
}
| private val db: ZeroCoinAppDatabase, | ||
| ) : LocalOrderBookRepository { | ||
|
|
||
| val scope = CoroutineScope(Dispatchers.IO) |
There was a problem hiding this comment.
Como buena practica convierte tus variables en privadas si solo las usas en ese archivo
Comment on lines
+25
to
+26
| val context = LocalContext.current | ||
| val listState = rememberLazyListState() |
There was a problem hiding this comment.
Variables que no se usan deben eliminarse
Comment on lines
+36
to
+38
| if (cryptoCoinsKeys.isEmpty()) CryptoCoinUI.crypto.coinKey else cryptoCoinsKeys[0] | ||
| val sellerCryptoCoinKey = | ||
| if (cryptoCoinsKeys.size == 2) cryptoCoinsKeys[1] else CryptoCoinUI.crypto.coinKey |
There was a problem hiding this comment.
Evita tener "números mágicos" en código, es mas recomendable pasalos a una constante
Comment on lines
+111
to
+123
| fun ErrorDialogHandler(show: Boolean) { | ||
| val context = LocalContext.current | ||
| var showCustomDialog by remember { | ||
| mutableStateOf(false) | ||
| } | ||
| showCustomDialog = show | ||
| if (showCustomDialog) { | ||
| ErrorDialog({ | ||
| showCustomDialog = !showCustomDialog | ||
| }, { | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Me parece que este componente nunca lo usas en el proy o si?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Se utilizó lint para arreglar codigo.
Añade inyección de dependencias
Cambia a navigation Component la navegación
Agrega la opción de usar RX para el servicio available order books
Agrega opcional mostrar LazyColumn de Compose en lugar de RecyclerView en la primera pantalla