[Team-02_Android] [Linus_Funny] 4번째 PR 요청 _ API 통신 + 드래그 앤 드롭 + 활동 기록 #212

cherrylime69 · 2022-04-15T06:54:56Z

작업 현황

API 통신을 통해 추가, 삭제, 드래그 앤 드롭 구현 완료
- 추가, 삭제, 드래그 앤 드롭에 대한 로직은 서버 측에서 담당
- 안드 팀은 추가, 삭제, 드래그 앤 드롭을 할때마다 서버에 요청하여 화면을 갱신함
드래그 앤 드롭 구현 완료
- 한 리사이클러뷰의 내부에서 이동 완료
- 다른 리사이클러뷰 간의 이동 완료
앱바의 navigation bar를 누르면 API 통신을 통해 표시
롱클릭하면 메뉴가 뜨도록 구현
메뉴에서 완료한 일을 누르면 해당 카드가 완료한 일로 가도록 구현
메뉴에서 수정을 누르면 해당 카드를 수정

문제점

swipe 와 드래그 이벤트 겹쳐서 옆으로 빠르게 swipe 하면 스와이프가 구현되고 천천히 움직이면 드래그가 구현됨
swipe 된 viewHolder 가 재활용될 때 swipe 되지 않도록 보완하기 실패
(저희 코드에서는 스크롤하여 viewHolder에 bind할 때 스와이프를 풀도록 함)

결과

질문

문제점 1에서 swipe 와 drag 이벤트가 겹치고 있는 점을 어떻게 하면 해결할 수 있는지 조언을 부탁드려도 될까요?
저희 팀은 리사이클러뷰 간의 이동을 할 때 공식문서 참고하여 드래그 앤 드롭을 구현했습니다. 하지만 해당 방식은 target으로 지정한 뷰내에서만 드롭을 할 수 있어 리사이클러뷰 간에 드래그 앤 드롭을 하는데 적합한가에 대해 의문이 남습니다. 혹시 cozi 라면 이 부분을 어떤 방식으로 구현할 것인지 키워드나 조언을 남겨주실 수 있을까요? 또는 현업에서는 어떻게 처리하고 있나요?

소감

Linus : 친절하게 알려주셔서 감사합니다. 확실히 저희가 생각하지 못한 부분이 있어 (프래그먼트에는 생성자 x 등) 많은 점을 배웠습니다. 저희가 기능을 구현하는데 급급하여 원활하게 소통하지 못한 점이 아쉽지만 혹시나 다음에 다시 리뷰를 받게 된다면 더 열심히 하도록 하겠습니다. 감사합니다 :D

Funny : 좋은 피드백 감사합니다. 모든 피드백을 반영해보고 싶었지만 저희의 실력 부족으로 모두 반영하지 못해 많이 아쉬웠습니다. 다음에는 네이버에서 직접 피드백을 받을 수 있도록 하겠습니다 ㅎㅎ

[funny] 피드백 반영, retrofit 적용 중

[funny] 피드백 반영과 레트로핏 적용중

[funny] network 추가 기능 구현

[funny, linus] 글 추가기능 Api 연동

delete와 drag, drop api 연결

[funny] 드래그앤드롭, 삭제 api 연결

롱클릭 팝업 메뉴 구현

[퍼니] 롱 클릭 팝업 메뉴 구현

- swipe 오류 수정 - 레이아웃 일부 보완

[퍼니 & 라니어스] 활동 기록 구현

Zimins

고생하셨습니다~

레이어를 여러 단계로 나누는 시도를 해주셨는데요, reposioty, dataSource 등을 잘 생각하고 구현을 해주셨으나 일부 구현에서 서버측의 변경 사항이 뷰모델 까지 올라올 가능성이 있습니다.

레이어 분리의 목적은 각 계층의 변경이 독립적으로 이루어 지는것에 있는데요. 아래 상황에서 관련 코드만 변경될 수 있도록 짜보면 이해가 좀 될수 있어보입니다 .
예시

어느날 특정한 이유로 retrofit 을 못쓰고 다른 라이브러리를 써야한다.
서버에서 주는 데이터 모델이 camel case 에서 snake_case 로 바뀌었다.
서버 데이터를 사용하는 쪽에서는 enum 을 활용하고 싶다.

리뷰 확인만 하시고 바로 머지가셔도 됩니다~

Zimins · 2022-04-17T09:35:53Z

android/todo-list/app/src/main/java/com/example/todolist/data/CardRemoteDataSource.kt

+    override suspend fun addCard(newCard: NewCard): Response<Card> {
+        return apiClient.addCard(newCard)
+    }


이 메소드에서 Card 를 리턴해야 하는 상황이 어떻게 될까요?
아마 이 카드를 추가 할 때는 이미 new card 라는 데이터가 있어서 보기에는 같은게 아닐까? 생각이 들었습니다.

Zimins · 2022-04-17T09:38:53Z

android/todo-list/app/src/main/java/com/example/todolist/data/CardRepository.kt

+    suspend fun getCards(): Data? {
+        val cards = cardDataSource.getCards()
+        return cards.body()?.data
+    }


getCards() 메소드를 사용하는 사람은 리턴값으로 List 를 기대할 확률이 높습니다. 또한, Data 는 서버쪽에서 주체를 가지는 형태로 보이는데요, Repository 등으로 레이어를 나누는 것은 서버는 어떻게 구현되는지 어떤 모양의 데이터를 사용하는지 모르게 하는것에도 목적이 있습니다.
포인트가 약간 감이 안 올수도 있는데, 두 리턴 타입을 놓고 장단점을 생각해보는 연습을 해보시면 좋겠습니다!

Zimins · 2022-04-17T09:39:20Z

android/todo-list/app/src/main/java/com/example/todolist/data/CardRepository.kt

+    suspend fun addCard(subject: String, content: String, status: String): Card? {
+        return cardDataSource.addCard(NewCard(subject, content, status)).body()
+    }


여기도 정말 리턴 타입이 꼭 필요한 상황인지 확인 부탁드립니다!

Zimins · 2022-04-17T09:42:56Z

android/todo-list/app/src/main/java/com/example/todolist/ui/CardViewModel.kt

+        val cards = cardRepository.getCards()
+        cards?.todo?.cards?.sortedByDescending { it.order }?.let { setTodoList(it) }
+        cards?.ongoing?.cards?.sortedByDescending { it.order }?.let { setOngoingList(it) }
+        cards?.completed?.cards?.sortedByDescending { it.order }?.let { setCompletedList(it) }


너무 많은 safe call 은 코드의 동작이 모호하거나 나중에 디버깅이 매우 어려울 수 있습니다. (관련된 변수중 하나라도 null 일 경우 아무 예외/메시지 없이 지나감)

이를 줄이는 방법에는 여러가지가 있습니다. 검사하는 부분을 앞쪽에 모아놓거나, 변수를 새로 도입하거나 등의 방식으로 가능하면 ? 를 줄여보면 좋겠습니다!

Zimins · 2022-04-17T09:45:02Z

android/todo-list/app/src/main/java/com/example/todolist/ui/CardViewModel.kt

+    override fun addCard(newCard: NewCard) {
+        viewModelScope.launch {
+            val card = cardRepository.addCard(newCard.subject, newCard.content, newCard.status)
+            when (card?.status) {


status 가 일반 문자열 대신 enum 이었으면 더 명확할수 있을 코드로 보입니다.
이런 타입의 경우 쓰는 쪽에서 매번 어떤 문자열이 올 수 있는지 다른 사람의 코드를 찾아다니면서 확인해야 하거든요.

위 목적을 달성하려면 네트워크용 클래스/앱 전용 클래스의 분리가 선행되어야 합니다. 앞으로 다른 과제도 진행해보시면서 고민해보시면 좋겠네요~

Zimins · 2022-04-17T09:47:22Z

android/todo-list/app/src/main/java/com/example/todolist/ui/CardViewModel.kt

+                val cardId = draggedCard.cardId?.let { it }
+                    ?: throw IllegalArgumentException("cardId is null!")


같은 역할의 코드를 requireNotNull 같은 함수를 사용하면 가독성 좋게 만들어 볼수도 있습니다.

실제로 줄어드는 코드는 얼마 안되지만, 아래쪽에 비슷한 코드들이 반복되므로 더 명확하게 보이는 효과가 있습니다.

Zimins · 2022-04-17T09:49:36Z

android/todo-list/app/src/main/java/com/example/todolist/ui/MainActivity.kt

+        val ongoingAdapter = CardAdapter(cardViewModel)
+        binding.rvOngoing.adapter = ongoingAdapter
+        val completedAdapter = CardAdapter(cardViewModel)
+        binding.rvCompleted.adapter = completedAdapter


adapter 에서는 되도록이면 뷰모델 의존성이 없는게 좋을때가 많습니다.
지금과 같은 구현은 뷰모델을 여러 단계의 레이어에서 건드리게 되므로, 무언가 변경이 매우 어려워질 확률이 큽니다.

밑에서 확인해보니 이미 개선해주셨군요 :)

Zimins · 2022-04-17T09:51:02Z

android/todo-list/app/src/main/java/com/example/todolist/ui/adapter/CardAdapter.kt

+class CardAdapter(
+    private val cardActionHandler: CardActionHandler
+) : ListAdapter<Card, CardAdapter.CardViewHolder>(DiffUtil) {


아, 이런식으로 뷰모델 의존을 한번 끊으셨군요!
이렇게 되면 뷰모델 의존성이 사라지므로 adapter 을 약간 더 자유롭게 사용 할 수 있습니다!

Zimins · 2022-04-17T10:05:18Z

질문 사항 답변

이벤트의 겹침
요건 기술적으로 해결하기보다 ux 자체가 변경되는게 좋아보입니다. trello 같은 칸반 보드 서비스에서 이런 방식을 어떻게 구현했는지 보면 좋을거 같아요. 뭐 아이폰/안드로이드 홈화면도 비슷하구요.
속도만으로 드래그/스와이프를 나누는 상황자체를 고쳐봅시다!

드래그 앤 드롭

일단 저도 실제 서비스에서 드래그드롭을 구현한적이 없어서 애매하지만, 약간의 의견을 첨부드릴게요.
드래그 하는 무언가가 들어가야 할 자리는 있어야 할거같아요. 아무 자리 없이 자유롭다면 이는 현재 구현 스펙과 다르기도 하고, colum 구조가 아닌 다른 형태의 앱이 되어야 할거같습니다.

물론 그래도 필요하다면, 아예 리사이클러뷰를 빼고 나만의 드래그 드롭이 가능한 커스텀 뷰를 새로 구현 할 듯 하네요. 커스텀 뷰를 만들듯이 커스텀 뷰그룹에 대한 학습을 하고 만들면 됩니다. 혹은 Canvas 를 사용하여 작은 부분부터 그리는 방법도 있습니다!

ese111 and others added 15 commits April 14, 2022 14:09

feat: 피드백 반영, retrofit 적용 중

41f60f7

Merge pull request #5 from ese111/feature-binding

68b79ee

[funny] 피드백 반영, retrofit 적용 중

Merge pull request #32 from ese111/main

307a0fe

[funny] 피드백 반영과 레트로핏 적용중

feat: network 추가 기능 구현

0a996eb

Merge pull request #6 from ese111/feature-binding

e1b042f

[funny] network 추가 기능 구현

Merge pull request #33 from ese111/main

0483bc7

[funny, linus] 글 추가기능 Api 연동

feat: 드래그앤드롭 api 연결

61daba2

Merge pull request #7 from ese111/feature-delete

383d202

delete와 drag, drop api 연결

Merge pull request #34 from ese111/main

51b89a3

[funny] 드래그앤드롭, 삭제 api 연결

feat: 터치 후 바로 드래그 시 카드 이동, 롱클릭시 메뉴 팝업 뜨는 기능 구현

6dbfeeb

Merge pull request #8 from ese111/feature-popup-menu

7b1207c

롱클릭 팝업 메뉴 구현

Merge pull request #35 from ese111/main

e772c8c

[퍼니] 롱 클릭 팝업 메뉴 구현

Design : layout 보완 및 클래스명 수정

0ac7a70

feat : 활동 기록 구현

bab08d9

- swipe 오류 수정 - 레이아웃 일부 보완

Merge pull request #38 from cherrylime69/dev-android-feature-contextMenu

78d3479

[퍼니 & 라니어스] 활동 기록 구현

astraum requested a review from Zimins April 15, 2022 09:42

astraum added the review-android Further information is requested label Apr 15, 2022

Zimins reviewed Apr 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Team-02_Android] [Linus_Funny] 4번째 PR 요청 _ API 통신 + 드래그 앤 드롭 + 활동 기록 #212

[Team-02_Android] [Linus_Funny] 4번째 PR 요청 _ API 통신 + 드래그 앤 드롭 + 활동 기록 #212

cherrylime69 commented Apr 15, 2022

Zimins left a comment

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins Apr 17, 2022

Zimins commented Apr 17, 2022

		val cardId = draggedCard.cardId?.let { it }
		?: throw IllegalArgumentException("cardId is null!")

[Team-02_Android] [Linus_Funny] 4번째 PR 요청 _ API 통신 + 드래그 앤 드롭 + 활동 기록 #212

Are you sure you want to change the base?

[Team-02_Android] [Linus_Funny] 4번째 PR 요청 _ API 통신 + 드래그 앤 드롭 + 활동 기록 #212

Conversation

cherrylime69 commented Apr 15, 2022

작업 현황

문제점

결과

질문

소감

Zimins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zimins commented Apr 17, 2022