feat: 채팅 view type 관리 및 날짜 view 추가 #588
Conversation
…hongdae-market into feature/501-chatting-refactor
| @@ -107,47 +90,49 @@ class CommentDetailViewModel | |||
| } | |||
| } | |||
|
|
|||
| private fun handleRepositoryError( | |||
There was a problem hiding this comment.
네이밍을 구체화 하는건 어떨까요??
예를 들면 handleNetworkError와 같이요!
There was a problem hiding this comment.
repository 에서 일어나는 error 여서 이렇게 표현을 했는데, 현재는 network error 밖에 처리를 하고 있지 않으므로 네이밍을 변경하겠습니다. 하지만 추후 local repository에서 일어나는 에러도 표현하기 위해 더 적합한 네이밍을 생각해봐야겠군요,.,.
|
|
||
| for (i in comments.indices) { | ||
| val currentComment = comments[i] | ||
| val previousComment = if (i > 0) comments[i - 1] else null |
There was a problem hiding this comment.
val previousComment = if (i != 0) comments[i - 1] else null
사소한 부분이긴 한데요, 이렇게 바꾸는 건 어떨까요?
첫번째 코멘트인지 아닌지를 구분하기 위함이다라는 걸 파악하기 쉬울 것 같아요~!
There was a problem hiding this comment.
첫번째 코멘트가 아닌 이전 comment가 있는 경우를 previous 로 설정하는 부분이라 로직상 i > 0가 맞는 것 같습니다!
| val currentComment = comments[i] | ||
| val previousComment = if (i > 0) comments[i - 1] else null | ||
|
|
||
| if (previousComment == null || currentComment.commentCreatedAt.date != previousComment.commentCreatedAt.date) { |
There was a problem hiding this comment.
조건문의 로직을 메서드 분리해서 네이밍을 통해 어떤 것을 검증하는 건지 알려주면 좋을 것 같아용
There was a problem hiding this comment.
분리하면 더 좋을 듯 하네요 반영했습니다~
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
|
|
||
| @SpringBootTest | ||
| class ChongdaeApplicationTests { | ||
|
|
||
| @Test | ||
| void contextLoads() { | ||
| } | ||
|
|
There was a problem hiding this comment.
develop 에서 바로 따왔는데 이상한 코드가 들어왔어요... 나중에 충돌날까바 그냥 뒀어요
| for (i in comments.indices) { | ||
| val currentComment = comments[i] | ||
| val previousComment = if (i > 0) comments[i - 1] else null | ||
|
|
||
| if (previousComment == null || currentComment.commentCreatedAt.date != previousComment.commentCreatedAt.date) { | ||
| newItems.add(CommentViewType.DateSeparator(currentComment)) | ||
| } | ||
|
|
||
| newItems.add(CommentViewType.fromComment(currentComment)) | ||
| } |
There was a problem hiding this comment.
forEachIndexed함수를 쓰는 것이 가독성이 더 좋을 것 같은데, 어떠신가요?
There was a problem hiding this comment.
오호 좋은 리뷰 감사해요!
forEachIndexed를 활용한다면 더 좋을 것 같은데, 큰 리스트를 다룰 때는 일반적인 for (i in indices)가 더 적합하다고 생각해서 이렇게 구현을 했습니다. 하지만 나중에 백친구들이 댓글에 페이징해준다면 forEachIndexed를 쓰는게 더 가독성이 좋겠네요!
| when (authRepository.saveRefresh()) { | ||
| is Result.Success -> retryAction() | ||
| is Result.Error -> | ||
| _event.value = | ||
| Event(CommentDetailEvent.ShowError("로그아웃 후 다시 진행해주세요.")) |
There was a problem hiding this comment.
와 함수분리 너무 좋아요! 토큰 리프레시 작업을 어떻게 함수분리 해야하나 하고 깝깝했는데 솔루션을 제시해 주셔서 고마워요~~
There was a problem hiding this comment.
"로그아웃 후 다시 진행해주세요." 문자열은 string resource로 빼고, ShowError 객체는 파라미터로 Int타입을 받고 액티비티에서 getString()을 통해서 문자열을 가져오는 방법이 좋을 것 같아요.
There was a problem hiding this comment.
그리고 이 함수는 exitOffering() 함수에도 적용 가능할 것 같은데, 맞나요? (혹시 놓친 것인지 아니면 적용하지 않은 의도가 있는 것인지 여쭤봅니당 😄)
There was a problem hiding this comment.
exitOffering에서는 특수한 상황으로 body 가 null 로 들어와서 error 로 처리되더라고요...
'Delete' API만 이렇게 작동하는 것을 이전에 백엔드와 얘기했는데, 결론을 말하자면 의도적으로 적용 안한겁니다!
There was a problem hiding this comment.
현재 error 부분이 String 으로 되어있어서 event 처리를 똑같이 주려고 이렇게 해놨습니다! 나중에 api 핸들러 다시 리팩토링 해야할듯합니다!
📌 관련 이슈
close #501
✨ 작업 내용
📚 기타
채팅을 어느 날짜에 보냈는지 알려주는 기능 추가했서요!!
