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

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 object로 ListAdapter에 사용될 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도 비교를 해야해서 Model의 DiffUtil을 사용할 수 없다는 것이 이유였다고 설명 했습니다.

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
        }
}

HomeItemModel의 isTheSame은 우선 상위 클래스인 Model의 isTheSame을 이용하여 서로 id와 type이 같은지 확인한 다음에 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.