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

DEP-175 feat: 습관 삭제 기능 구현 #53

Merged
merged 10 commits into from
Dec 15, 2022

Conversation

juhwankim-dev
Copy link
Member

💁‍♂️ 변경 내용

KakaoTalk_20221204_002749813

AS-IS

  • 삭제하기 버튼을 눌러도 아무 일이 일어나지 않았습니다.
  • 삭제 api가 연동되어있지 않았습니다.
  • 일부 파일에서 habitId가 long 타입으로 되어있었습니다.
  • viewModel로부터 event를 전달 받는 수단/경로가 없었습니다.

TO-BE

  • 삭제하기 버튼을 누르면 다이얼로그가 뜹니다.
  • 삭제 api를 연동했습니다. (mock 서버입니다)
  • habitId를 Int형으로 통일했습니다.
  • SharedFlow를 통해 viewModel로부터 event를 전달 받습니다.

📢 전달사항

  • 현재 삭제하기 버튼을 누르면 앱이 종료됩니다. (그럼에도 pr 올린 이유는 이를 여쭤보기 위함입니다)
    20221204_003317
    삭제 성공 시 돌아오는 데이터가 없는데 이걸 어떤 형으로 받아야 할까요..?
    Any로 받아봤는데 앱이 자꾸 죽네요.
    이것저것 시도해보다가 뭔가 바보 같은 짓을 하고 있는 것 같은데 모르겠어서 질문 드립니다. 🥲

그리고 BaseResponse가 필요할지 의견을 여쭤보고 싶습니다.

data class BaseResponse (
    val data?: T,
    val code?: String,
    val message?: String
)

대충 이런 느낌으로...
성공할 때랑 실패할 때 돌아오는 데이터의 유형이 달라서 어떻게 해야 할 지 잘 모르겠네요 😭😭

@juhwankim-dev juhwankim-dev added the enhancement New feature or request label Dec 3, 2022
@juhwankim-dev juhwankim-dev requested a review from a team as a code owner December 3, 2022 15:41
@juhwankim-dev juhwankim-dev self-assigned this Dec 3, 2022
@junhaesung
Copy link
Member

junhaesung commented Dec 4, 2022

성공/실패 다를 때 각각 다른 클래스로 모델 매핑하는 방법이 있는지 찾아봐야겠네요.
아니면 동일하게 성공시에도 data, code, message 가지는 최상위 모델 하나 추가해달라고 하는 것도 방법일거같아요.


body 가 없으면 Void 로 받으면 되지않을까요? ㅋㅋㅋ 저도 찾아보겠습니다..


habitId 는 Long 으로 써야할거같아요!
서버에서 Long 범위로 만들고 있어서 우리가 int 쓰면 파싱 실패하거나 overflow 발생할수도 있어서요

Copy link
Contributor

@kimhyeing kimhyeing left a comment

Choose a reason for hiding this comment

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

서버 측에 부탁해서 형태를 고정하는 게 저는 좀 더 편할 것 같네용!(data, code, message)
그런데 code를 좀 더 구체적으로 명시해놓으셨는데 문자열로 받는게 더 맞는?? 방법인건가요?
(잘 몰라서..)
200, 400, 500 등으로 받다가 문자로 받으니 어색한 느낌? 은 있네요 (나중에 에러처리할 때도 문자열로 구분해서 처리해야하나 싶은...)

Comment on lines +46 to +53
if(response != null) {
emit(DataState.success(data = response))
} else {
emit(DataState.error(msg = "response has error"))
}.runCatching {
emit(DataState.fail("response is fail"))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

서버 측에 똑같이 data, code, message를 동일한 형태로 달라고 하고 code등으로 400번대 에러, 500번대 에러, 200번대 성공으로 구분하는게 더 좋아보이는데.. 지금 형식은 response가 오면 에러고 오지않으면 성공이라는게 모호하게 구분되는 느낌이긴 하네요 !!

Copy link
Member Author

Choose a reason for hiding this comment

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

공감합니다! (카톡에서도 언급해주셔서 카톡으로 이어서 이야기할게요)

onPositiveAction = {
viewModel.deleteGoals(habitId)
},
emoji = getString(R.string.wastebasket),
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 제가 Emoji 클래스를 만들어놔서 함수로 가져올 수 있게 해놓긴 했는데 편하신대로 작성하셔도 될 것 같슴니당

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 맞네요 이건 수정하겠습니다.

@juhwankim-dev
Copy link
Member Author

성공/실패 다를 때 각각 다른 클래스로 모델 매핑하는 방법이 있는지 찾아봐야겠네요. 아니면 동일하게 성공시에도 data, code, message 가지는 최상위 모델 하나 추가해달라고 하는 것도 방법일거같아요.

body 가 없으면 Void 로 받으면 되지않을까요? ㅋㅋㅋ 저도 찾아보겠습니다..

habitId 는 Long 으로 써야할거같아요! 서버에서 Long 범위로 만들고 있어서 우리가 int 쓰면 파싱 실패하거나 overflow 발생할수도 있어서요

  1. 최상위 모델을 추가해주면 편할 것 같긴 한데 이게 백엔드 입장에서 귀찮은 작업이 될까요? 🤔
    방법 있는지는 저도 찾아보겠습니다.

  2. 오 그럼 코틀린에는 void가 없으니 unit으로...! 이것도 한 번 해볼게요

  3. 엇 swagger 문서에서 int형 사용하고 있던데 한 번 여쭤봐야겠네욤 감사합니다.

@juhwankim-dev
Copy link
Member Author

서버 측에 부탁해서 형태를 고정하는 게 저는 좀 더 편할 것 같네용!(data, code, message) 그런데 code를 좀 더 구체적으로 명시해놓으셨는데 문자열로 받는게 더 맞는?? 방법인건가요? (잘 몰라서..) 200, 400, 500 등으로 받다가 문자로 받으니 어색한 느낌? 은 있네요 (나중에 에러처리할 때도 문자열로 구분해서 처리해야하나 싶은...)

code는 swagger에 string으로 되어 있길래 저도 string으로 명시해두었습니다.
code라고 하면 뭔가 말씀하신 것 처럼 200 300 같은 숫자가 넘어올 것 같이 생긴 이름인데
문자열이 넘어오니 어색하긴 하네요..!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@juhwankim-dev juhwankim-dev merged commit 46882d4 into develop Dec 15, 2022
@juhwankim-dev juhwankim-dev deleted the DEP-175_delete_habit_funtion branch December 15, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants