[Team 박카스] 팀 프로젝트 2022.01.24 코드 리뷰

ErroredPasta·2022년 2월 2일
0

코드리뷰

목록 보기
1/4

Team 박카스는 현재 5인으로 구성된 팀으로 MVVM 아키텍처를 이용한 안드로이드 팀 프로젝트를 진행 중입니다. 해당 프로젝트는 아래의 GitHub 링크에서 확인해 보실 수 있습니다.

프로젝트 GitHub Repository

팀 프로젝트에서 서로 맡은 부분을 서로 구현하여 commit을 하면 나머지 팀원들이 해당 내용을 검토하여 개선할 부분이 있는지 코드 리뷰를 하는 방식으로 진행하고 있습니다.

첫번째 코드 리뷰 내용

위 링크는 첫번째 코드 리뷰에서 어떤 코드를 다루었는지 작성한 글이고 해당 글에서는 제가 어떤 점을 개선하였는지 작성할 예정 입니다.

발견된 문제점

가장 눈에 띄게 발견된 문제점은 본 프로젝트에서는 RecyclerView에 사용할 adapter를 ModelRecyclerAdapter 클래스 하나만 구현하여 사용하고 있었습니다.

class ModelRecyclerAdapter<M : Model, VM : BaseViewModel>(
    private var modelList: List<Model>,
    private val viewModel: VM,
    private val resourcesProvider: ResourcesProvider,
    private val adapterListener: AdapterListener
) : ListAdapter<Model, ModelViewHolder<M>>(Model.DIFF_CALLBACK) {

    override fun getItemViewType(position: Int): Int = modelList[position].type.ordinal

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ModelViewHolder<M> {
        return ViewHolderMapper.map(parent, CellType.values()[viewType], viewModel, resourcesProvider)
    }

    @Suppress("UNCHECKED_CAST")
    override fun onBindViewHolder(holder: ModelViewHolder<M>, position: Int) {
        holder.bindData(modelList[position] as M)
        holder.bindViews(modelList[position] as M, adapterListener)
    }

    override fun submitList(list: List<Model>?) {
        list?.let { modelList = it }
        super.submitList(list)
    }
}

나타낼 데이터를 Model을 상속받은 클래스를 이용하여 나타냅니다. 해당 클래스 내에는 CellType이라는 enum property가 존재하며 이는 ViewHolderMapper에서 어떤 ViewHolder를 생성할지 구분할 때 사용되고 companion objectListAdapter에 사용될 DiffUtil을 가지고 있습니다.

abstract class Model(
    open val id : Long,
    open val type : CellType
) {
    companion object {
        val DIFF_CALLBACK = object: DiffUtil.ItemCallback<Model>() {
            override fun areItemsTheSame(oldItem: Model, newItem: Model): Boolean {
                return oldItem.id == newItem.id && oldItem.type == newItem.type
            }

            @SuppressLint("DiffUtilEquals")
            override fun areContentsTheSame(oldItem: Model, newItem: Model): Boolean {
                return oldItem === newItem
            }

        }
    }
}

이렇게 하나의 adapter 클래스만 구현하여 프로젝트에서 사용하려 했으나 코드 리뷰를 할 때 거의 유사한 adapter 클래스가 하나 더 구현된 것을 발견했습니다.
이를 구현한 팀원에게 adapter 클래스를 하나 더 구현한 이유가 무엇인지 물었을 때, 자신이 구현한 부분에서 사용될 HomeListModel에서 두 아이템이 같은지 비교할 때 homeListCategory라는 enum도 비교를 해야해서 ModelDiffUtil을 사용할 수 없다는 것이 이유였다고 설명 했습니다.

abstract class HomeListModel(
    override val id: Long,
    override val type: CellType = CellType.HOME_CELL,
    open val homeListCategory: HomeListCategory
) : Model(id, type) {

    companion object {
        val DIFF_CALLBACK: DiffUtil.ItemCallback<HomeListModel> =
            object : DiffUtil.ItemCallback<HomeListModel>() {
                override fun areItemsTheSame(
                    oldItem: HomeListModel,
                    newItem: HomeListModel
                ): Boolean {
                    return oldItem.id == newItem.id && oldItem.type == newItem.type && oldItem.homeListCategory == newItem.homeListCategory
                }

                @SuppressLint("DiffUtilEquals")
                override fun areContentsTheSame(
                    oldItem: HomeListModel,
                    newItem: HomeListModel
                ): Boolean {
                    return oldItem === newItem
                }
            }
    }
}

비교 조건이 다르다고 클래스를 계속 생성하게 되는건 바람직하지 않다고 생각하여 해당 문제에 대해 해결책을 생각하게 되었습니다.

해결책

이 문제를 해결하기 위해서는 하나의 DiffUtil에서 조건이 다르더라도 두 아이템이 같은 아이템인지 비교할 수 있는 방법이 필요했습니다. 그래서 Model 클래스에서 두 아이템이 같은지 판단하는 isTheSame이라는 method를 구현하여 DiffUtil에서 해당 method를 이용하여 아이템이 서로 같은지 비교하도록 하였습니다.

abstract class Model(
    open val id : Long,
    open val type : CellType
) {
    open fun isTheSame(item: Model) =
        this.id == item.id && this.type == item.type

    companion object {
        val DIFF_CALLBACK = object: DiffUtil.ItemCallback<Model>() {
            override fun areItemsTheSame(oldItem: Model, newItem: Model): Boolean {
                return oldItem.isTheSame(newItem)
            }

            @SuppressLint("DiffUtilEquals")
            override fun areContentsTheSame(oldItem: Model, newItem: Model): Boolean {
                return oldItem === newItem
            }
        }
    }
}

이렇게 구현하면 비교 조건이 다를 때 Model을 상속 받는 클래스에서 isTheSame method를 override하여 다른 조건을 사용할 수 있게 됩니다. 이를 이용하여 프로젝트에 적용한 내용은 아래와 같습니다.

data class HomeItemModel(
    override val id: Long,
    val homeListCategory: HomeListCategory,
    ... // Parameters
    override val type: CellType = CellType.HOME_ITEM_CELL
): Model(id, type) {

    override fun isTheSame(item: Model) =
        if (item is HomeItemModel) {
            super.isTheSame(item) && this.homeListCategory == item.homeListCategory
        } else {
            false
        }
}

HomeItemModelisTheSame은 우선 상위 클래스인 ModelisTheSame을 이용하여 서로 idtype이 같은지 확인한 다음에 homeListCategory가 같은지 확인을 하게 됩니다. 이를 이용하여 같은 아이템인지 확인하는 method를 구현하여 adapter를 최소한으로 구현하여 RecyclerView를 사용할 수 있게 되었습니다.

다른 개선할 점

val homeListLiveData = when(homeListCategory) {
        HomeListCategory.TOWN_MARKET -> MutableLiveData<List<TownMarketModel>>()
        else -> MutableLiveData<List<HomeItemModel>>()
    }

코드에서 조건문을 작성할 때 binary condition에 대해 when문으로 작성한 것을 발견했습니다.

Prefer using if for binary conditions instead of when. [1]

하지만 binary condition일 경우에는 Kotlin 공식 페이지에서 coding convension으로 when이 아니라 if를 쓸 것을 권장하고 있습니다.

Reference

[1] "Coding conventions," Kotlin Programming Language, last modified Jan 14, 2022, accessed Feb 02, 2022, https://kotlinlang.org/docs/coding-conventions.html#if-versus-when.

profile
Hola, Mundo

0개의 댓글