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

Iteration 3 review #132

Merged
merged 114 commits into from
Aug 23, 2024
Merged

Iteration 3 review #132

merged 114 commits into from
Aug 23, 2024

Conversation

G-dev-ui
Copy link
Collaborator

https://github.com/users/ElchinGasymov/projects/1/views/1
Ссылка на доску
Все поставленные задачи Epic 4,5 выполнены
Количество людей в команде: 5
Первоначальная оценка времени 7 дней, затраченной время 7 дней

ElchinGasymov and others added 30 commits August 14, 2024 09:31
Приведение в порядок
И созданы начальные фрагменты
… работы (страна, регион)

А также поведение поля "Ожидаемая зарплата на экране фильтров."
Добавил новые suspend-методы классы Request и Response DTO модели м и тд
Интерактор для фильтров и di
Copy link

@ArturNurtdinov ArturNurtdinov left a comment

Choose a reason for hiding this comment

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

  1. Длинная отрасль обрезается перед крестиком:
    image

) : SharedPrefsRepository {
override suspend fun readSharedPrefs(): SaveFiltersSharedPrefs? {
val json = sharedPreferences.getString(HISTORY, null) ?: return null
/*SaveFiltersSharedPrefs(

Choose a reason for hiding this comment

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

Закоментированный код лучше сразу удалять. Хранить закоментированный код в проекте - плохая практика, в случае чего этот код можно восстановить с помощью истории гита

Comment on lines +69 to +78
val newList = ArrayList<Industries>()
list.value.forEach { industryItem ->
if (industryItem.id == industry.id) {
newList.add(industryItem.copy(isChecked = true))
_selectedIndustry.postValue(industryItem)
_hasSelected.postValue(true)
selectedId = industryItem.id
} else {
newList.add(industryItem)
}
Copy link

@ArturNurtdinov ArturNurtdinov Aug 23, 2024

Choose a reason for hiding this comment

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

Здесь можно newList заменить на mapNotNull, чтобы не создавать новый мьютабельный список

Comment on lines +87 to +101
val sortedList = mutableListOf<Industries>()
val newSortedList = mutableListOf<Industries>()
sortedList.addAll(listOfIndustries)
sortedList.removeAll {
!it.name.contains(request, true)
}
sortedList.forEach {
if (it.id == selectedId) {
newSortedList.add(Industries(it.id, it.name, true))
_selectedIndustry.postValue(it)
_hasSelected.postValue(true)
} else {
newSortedList.add(it)
}
}

Choose a reason for hiding this comment

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

Здесь тоже лучше через map сделать, а не через копирование списка и его модификацию, либо filter, либо map

}

fun getCountryName(region: Region, isSaving: Boolean) {
viewModelScope.launch(Dispatchers.IO) {

Choose a reason for hiding this comment

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

переключение потоков на IO должен выполнять репозиторий, а не вьюмодель. Вьюмодель не должна знать, из какого источника приходят данные

}

private fun onItemClicked(country: Country) {
val json = Gson().toJson(country)

Choose a reason for hiding this comment

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

Gson - тяжёлый объект, его лучше не создавать так часто, а создать такой объект один раз и переиспользовать

import ru.practicum.android.diploma.util.REGION_REQUEST_KEY

class FilterPlaceOfWorkFragment : Fragment() {
private val binding: FragmentSelectPlaceOfWorkBinding by viewBinding(CreateMethod.INFLATE)

Choose a reason for hiding this comment

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

При использовании viewBinding во фрагментах обязательно нужно обнулять _binding в onDestroyView, иначе будет утечка памяти. Почитать о том, как использовать эту фичу во фрагментах можно тут - https://developer.android.com/topic/libraries/view-binding#fragments. Почитать подробнее про утечку можно здесь - https://stackoverflow.com/questions/65295104/android-view-binding-clear-binding-in-fragment-lifecycle

Choose a reason for hiding this comment

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

Не стоит константы хранить в одном общедоступном месте, их следует хранить максимально близко к месту использования и с максимально ограниченной видимостью, желательно private

@G-dev-ui G-dev-ui merged commit 45d4357 into main Aug 23, 2024
3 checks passed
@ElchinGasymov ElchinGasymov deleted the iteration_3_review branch August 25, 2024 06:29
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.

6 participants