-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat [#172] Amplitude Setting 및 적용 완료 #173
Conversation
isa = XCSwiftPackageProductDependency; | ||
package = 3CFB30452BB305EE00A3F70A /* XCRemoteSwiftPackageReference "Amplitude-iOS" */; | ||
productName = Amplitude; | ||
}; | ||
/* End XCSwiftPackageProductDependency section */ | ||
}; | ||
rootObject = 3C6192E12B3A719A00220CEB /* Project object */; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치의 요약:
- 새로운 파일
Amplitude
가 추가되었습니다. - Amplitude는 Frameworks에 추가되었으며, 이에 대한 설정이 변경되었습니다.
- Amplitude-iOS가 XCRemoteSwiftPackageReference로 추가되었습니다.
개선 제안:
- 코드 변경 전 후의 테스트를 수행하여 새로운 구성의 정상 작동을 확인하세요.
- 각 파일 및 의존성에 적절한 주석을 추가하여 가독성을 향상시켜주세요.
- Swift 패키지들의 버전 호환성을 확인하고 업데이트해야 할 필요가 있습니다.
"revision" : "e2ca17ac735bcbc48b13062484541702ef45153d", | ||
"version" : "1.0.3" | ||
} | ||
}, | ||
{ | ||
"identity" : "kakao-ios-sdk", | ||
"kind" : "remoteSourceControl", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치에는 몇 가지 문제가 있습니다.
"branch"
키가 "main"으로 설정되어 있지만, 최신 버전의 git에서는 기본 브랜치 이름이 "main"이 아니라 "master"로 변경되었습니다. 따라서 "main" 대신 "master"를 사용하는 것이 좋습니다."revision"
필드가 한 번은"d4a71f3e06b5108696ca7b33567d0fc539597d82"
로 나타나고 다른 한 번은"e2ca17ac735bcbc48b13062484541702ef45153d"
로 나타납니다. 이러한 일관성 부족은 유지 보수 및 버그 해결을 어렵게 할 수 있습니다. 모두 같은 형식으로 통일하는 것이 좋습니다.
위의 내용들을 고려하여 코드를 수정하시기 바랍니다.
@@ -26,6 +27,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate { | |||
|
|||
KakaoSDK.initSDK(appKey: Config.nativeAppKey) | |||
|
|||
/// Amplitude 설정 | |||
Amplitude.instance().initializeApiKey(Config.amplitudeAppKey) | |||
|
|||
NotificationCenter.default.addObserver(forName: ASAuthorizationAppleIDProvider.credentialRevokedNotification, object: nil, queue: nil) { (Notification) in | |||
// 앱 실행 중 강제로 연결 취소 시 로그인 페이지로 이동 | |||
if let sceneDelegate = UIApplication.shared.connectedScenes.first?.delegate as? SceneDelegate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amplitude를 추가한 코드 패치에 대한 간단한 코드 리뷰:
-
장점:
- Amplitude를 initialize하고 API 키를 설정하는 부분이 보입니다.
- 주석을 통해 각 라이브러리의 역할 및 설정 목적을 명확하게 설명했습니다.
-
개선 사항:
Amplitude.initializeApiKey()
메서드가 콜백을 반환할 수 있으므로 초기화 작업 완료를 처리하는 것이 좋습니다.- 예외 처리나 오류 처리 부분이 보이지 않으므로, 이를 추가하여 안정성을 개선할 수 있습니다.
- 일관된 스타일을 유지하기 위해 다른 라이브러리 설정과 일치하도록 코드 스타일을 조정할 수 있습니다.
- 해당 애플리케이션의 다른 부분에서 사용되는 예시와 같은 디자인 패턴이나 코드 구조에 따라 Amplitude 통합을 개선할 수 있습니다.
-
잠재적 버그 리스크:
- UIApplication.shared.connectedScenes.first?.delegate 부분이 nil을 반환할 수 있으므로 optional chaining을 사용하는 것이 좋습니다.
- 옵저버가 중복으로 추가되는 경우를 방지하기 위해 addObserver를 호출하기 전에 removeObserver를 호출하거나 옵저버를 한번만 추가하도록 수정하는 것이 좋습니다.
-
기타 고려사항:
- 코드 가독성을 높이기 위해 Amplitude 설정과 관련된 주석을 더 자세히 달아 설명하는 것도 좋습니다.
- 클래스 또는 메서드 이름에 맞게 명명 규칙을 준수하여 의도를 더 잘 드러낼 수 있습니다.
- 팀 내에서 일관된 코드 스타일을 유지하고 문서화된 내용과 일치하도록 유지하는 것이 좋습니다.
@@ -22,6 +22,8 @@ | |||
</array> | |||
<key>NATIVE_APP_KEY</key> | |||
<string>$(NATIVE_APP_KEY)</string> | |||
<key>AMPLITUDE_APP_KEY</key> | |||
<string>$(AMPLITUDE_APP_KEY)</string> | |||
<key>NSAppTransportSecurity</key> | |||
<dict> | |||
<key>NSAllowsArbitraryLoads</key> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰:
- AMPLITUDE_APP_KEY가 추가되었지만 사용 전에 해당 값을 올바르게 설정했는지 확인해야 합니다.
- NSAppTransportSecurity 설정이 완전하지 않을 수 있으므로, 더 엄격한 보안 정책이 필요한지 고려할 필요가 있습니다.
isa = XCSwiftPackageProductDependency; | ||
package = 3CFB30452BB305EE00A3F70A /* XCRemoteSwiftPackageReference "Amplitude-iOS" */; | ||
productName = Amplitude; | ||
}; | ||
/* End XCSwiftPackageProductDependency section */ | ||
}; | ||
rootObject = 3C6192E12B3A719A00220CEB /* Project object */; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 리뷰에서 발견된 잠재적인 버그 위험:
MARKETING_VERSION
이 1.0.0에서 1.0.1로 변경되었지만, 이 업데이트에 대한 주석 또는 변경 사항 설명이 없습니다.- 빌드 파일 및 프로젝트 구성에 Amplitude 추가되면서, 해당 종속성이 적절히 관리되고 있는지 확인이 필요합니다.
repositoryURL = "https://github.com/amplitude/Amplitude-iOS";
와 같이 외부 종속성 링크를 사용할 때, 네트워크 및 소스 안정성 문제에 유의해야 합니다.
개선 제안:
- 변경된 버전(예: MARKETING_VERSION)에 대한 명확한 주석 추가.
- 변경사항을 충분히 문서화하여 다른 팀원들이 코드 변경사항을 이해하도록 하기.
- 외부 종속성 관리를 개선하기 위해, 주기적으로 코드 베이스를 검토하고 관련 업데이트가 있으면 업데이트하는 프로세스를 설정할 것.
코드 리뷰를 통해 더 나은 협업과 유지보수 가능한 코드베이스를 유지할 수 있도록 하세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3
상우 짱짱 bb 고생했습니다!
👻 PULL REQUEST
💻 작업한 내용
💡 참고사항
AppDelegate 파일 안에 앰플리튜드 키 값 넣어서 초기화 설정
Don-tBe-iOS/DontBe-iOS/DontBe-iOS/Application/AppDelegate.swift
Lines 20 to 32 in 2b275f5
아래와 같이 앰플리튜드 코드를 필요한 곳에 삽입해주면 됩니다!!
Don-tBe-iOS/DontBe-iOS/DontBe-iOS/Presentation/Write/ViewModels/WriteViewModel.swift
Lines 28 to 48 in 2b275f5
📟 관련 이슈