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

[#3] 지역코드 조회 API Service 추가 #4

Merged
merged 6 commits into from
Oct 1, 2024
Merged

[#3] 지역코드 조회 API Service 추가 #4

merged 6 commits into from
Oct 1, 2024

Conversation

ksw4015
Copy link
Collaborator

@ksw4015 ksw4015 commented Sep 26, 2024

변경사항

  • Hilt를 사용하기 위한 App class를 추가하였습니다.
  • Retrofit과 요청 Url에 공통 Query를 추가하기 위한 Interceptor class를 추가하였습니다.
  • 지역코드 조회 API 서비스를 추가하였습니다. (AreaCodeApi)
  • 응답에서 필요한 Data로 Mapping하기 위한 Mapper class를 추가하였습니다.
  • SplashActivity의 checkPermission 로직을 ViewModel을 만들어 옮겼습니다.

멘토님께

  • Api 응답이 복잡해서 Model class가 많습니다. 😓
  • Error응답은 xml 형식으로만 제공하여 따로처리하는 코드를 작성하지 못했습니다. 😭

추가정보

  • 응답 json data 추가합니다.
{
  "response": {
    "header": {
      "resultCode": "0000",
      "resultMsg": "OK"
    },
    "body": {
      "items": {
        "item": [
          {
            "rnum": 1,
            "code": "1",
            "name": "서울"
          },
          {
            "rnum": 2,
            "code": "2",
            "name": "인천"
          },
          {
            "rnum": 3,
            "code": "3",
            "name": "대전"
          },
          {
            "rnum": 4,
            "code": "4",
            "name": "대구"
          },
          {
            "rnum": 5,
            "code": "5",
            "name": "광주"
          },
          {
            "rnum": 6,
            "code": "6",
            "name": "부산"
          },
          {
            "rnum": 7,
            "code": "7",
            "name": "울산"
          },
          {
            "rnum": 8,
            "code": "8",
            "name": "세종특별자치시"
          },
          {
            "rnum": 9,
            "code": "31",
            "name": "경기도"
          },
          {
            "rnum": 10,
            "code": "32",
            "name": "강원특별자치도"
          }
        ]
      },
      "numOfRows": 10,
      "pageNo": 1,
      "totalCount": 17
    }
  }
}

@ksw4015 ksw4015 self-assigned this Sep 26, 2024
Copy link

@f-lab-Toru f-lab-Toru left a comment

Choose a reason for hiding this comment

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

Review completed

@@ -39,6 +48,7 @@ android {
jvmTarget = "17"

Choose a reason for hiding this comment

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

JavaVersion.VERSION_17.toString() 을 콜하면 "17" 이런 식으로 버전을 하드코딩하지 않아도 됩니다 :)

val properties = Properties().apply {
load(FileInputStream(rootProject.file("local.properties")))
}

android {
namespace = "kr.ksw.visitkorea"
compileSdk = 34

Choose a reason for hiding this comment

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

여담인데, 35가 곧 배포될 예정이라서 compile / target sdk version up 을 34로 유지하는 건 앞으로 1년 정도만 허용될 거예요. (지금 당장은 하지 않아도 됨)

@@ -7,6 +7,7 @@
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

<application
android:name=".App"

Choose a reason for hiding this comment

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

app 패키지를 따로 만들고, VisitKorea.kt 이런 식으로 Application 클래스 이름을 바꾸면 좀더 보기 좋지 않을까 하는 생각이 듭니다.


data class AreaCodeDTO(
val code: String,
val name: String

Choose a reason for hiding this comment

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

의견이 분분한 부분인데, trailing comma 를 넣어 줘도 좋을 거 같습니다.

@InstallIn(SingletonComponent::class)
object DataModule {

private const val BASE_URL = "https://apis.data.go.kr/B551011/KorService1/"

Choose a reason for hiding this comment

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

BASE_URL 과 같은 상수는 DataModule 에 둬도 좋지만, 상수가 많아진다면 상수를 관리하는 object 를 하나 둬도 괜찮을 거 같아요.

import kr.ksw.visitkorea.data.remote.model.CommonItem
import kr.ksw.visitkorea.data.remote.model.CommonResponse

class FakeAreaCodeRepository : AreaCodeRepository {

Choose a reason for hiding this comment

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

이전 회사에서 쓰던 방법인데, fake body json 을 파일로 하나 만들어 두고 그걸 읽어서 mock response 를 만들 수도 있습니다. 고려해 보세요. :)


@Test
fun `getSigunguCode request success, valid area code`() = runTest {
// val areaCodeItems = fakeAreaCodeRepository.getSigunguCode("1").getOrNull()

Choose a reason for hiding this comment

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

사용되지 않을 부분을 주석처리했다면 없애도 좋을 거 같아요

@@ -0,0 +1,5 @@
package kr.ksw.visitkorea.data.remote.model

data class ApiResponse<T>(

Choose a reason for hiding this comment

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

data class 의 마지막 item 들에는 trailing comma 를 넣어 주는 것이 좋습니다. 아래의 다른 data class 들도 마찬가지로요 :)

@@ -0,0 +1,6 @@
package kr.ksw.visitkorea.data.remote.model

data class CommonResponse<T>(

Choose a reason for hiding this comment

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

제네릭을 이용한 Response class 를 만드는 건 좋습니다. 그리고 이런 형태가 반복적으로 사용된다면 data/remote 가 아닌 common 과 같은 패키지를 하나 만들어서 관리해도 좋을 거 같아요.

private val _sideEffect = MutableSharedFlow<SplashSideEffect>(replay = 0)
val sideEffect: SharedFlow<SplashSideEffect> = _sideEffect.asSharedFlow()

fun checkPermission(activity: ComponentActivity) {

Choose a reason for hiding this comment

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

퍼미션 체크를 ViewModel 에서 하고 있는 것은 좋습니다.
checkPermission()  의 argument 가 activity 인데, context로 교체 가능하다면, AndroidViewModel 을 상속하는 걸 고려해 보는 건 어떨까요? 그러면 activity 를 쓰지 않아도 되어서 activity 와의 약한 coupling 이 제거될 수 있을 거 같아요.

@ksw4015
Copy link
Collaborator Author

ksw4015 commented Sep 30, 2024

Review 사항 일부 적용했습니다. AndroidViewModel로 변경하려했으나, registerForActivityResult가 Application이 아닌 ComponentActivity의 함수라 일단 유지하려고 합니다.

Copy link

@f-lab-Toru f-lab-Toru left a comment

Choose a reason for hiding this comment

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

리뷰 하나 더 남겼습니다.

Manifest.permission.ACCESS_COARSE_LOCATION,
Manifest.permission.ACCESS_FINE_LOCATION
))
private fun observeSideEffect() {

Choose a reason for hiding this comment

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

observeSideEffect() 라는 메소드명은 side effect 의 관찰에만 너무 중점을 두고 있는 느낌이라, observePermissionGrantResult 이런 식으로 바꿔도 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 알겠습니다!

@ksw4015 ksw4015 merged commit 4ea285a into main Oct 1, 2024
1 check failed
@ksw4015 ksw4015 deleted the feature/3 branch October 7, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants