Conversation
유저 이름 조회, 유저 이름 수정, 유저 탈퇴
MockuserDefaultsService -> MockUserDefaultsService
scenarios -> notifications
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 앱의 데이터 지속성 계층을 CoreData로 전환하여 핵심 데이터 관리 방식을 재정의합니다. 기존의 네트워크 기반 인증 및 멤버 관리 시스템을 로컬 CoreData 저장소로 대체함으로써, 시나리오, 미션, 약관, 멤버 정보를 효율적으로 관리할 수 있게 됩니다. 이는 로그인 흐름을 간소화하고 외부 소셜 로그인 의존성을 제거하며, 새로운 저장소 계층에 대한 광범위한 단위 테스트를 포함하여 데이터 관리의 견고함을 향상시킵니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 Core Data를 사용하여 로컬 데이터 영속성을 구현하는 중요하고 잘 수행된 리팩토링입니다. CoreDataStack, 다양한 스토리지 레포지토리(MemberStorage, ScenarioStorage 등)의 추가와 새로운 레이어에 대한 포괄적인 단위 테스트는 인상적입니다. 특히 관심사 분리와 Core Data의 perform 블록과 함께 async/await를 사용한 점 등 코드가 잘 구조화되어 있습니다.
하지만 몇 가지 중요한 수정 사항이 있습니다. MemberStorage의 의존성 주입에서 NSManagedObjectContext가 누락되어 런타임 크래시를 유발할 수 있습니다. 또한 ScenarioStorage에서 새로운 미션이 중복된 ID(0)로 생성될 수 있는 잠재적인 버그가 있습니다. 주석 처리된 코드 제거 및 업데이트 메서드의 효율성 개선과 같은 몇 가지 사소한 개선 사항에 대한 제안도 남겼습니다.
전반적으로 애플리케이션의 데이터 레이어를 견고하게 구축하는 데 있어 훌륭한 진전입니다.
| // userDefaultsService: userDefaultsService, | ||
| // updateNicknameRequestMapper: updateNicknameRequestMapper | ||
| // ) | ||
| MemberStorage(userDefaultsService: userDefaultsService) |
There was a problem hiding this comment.
MemberStorage를 초기화할 때 NSManagedObjectContext가 누락되었습니다. MemberStorage의 init은 context를 필요로 하므로, 이대로는 런타임에 크래시가 발생할 것입니다. CoreDataStack.shared.context를 주입해야 합니다.
| MemberStorage(userDefaultsService: userDefaultsService) | |
| MemberStorage(userDefaultsService: userDefaultsService, context: CoreDataStack.shared.context) |
| id: Int? = nil | ||
| ) { | ||
| let mission = Mission(context: context) | ||
| mission.id = Int64(id ?? 0) |
There was a problem hiding this comment.
createMission 함수에서 새로운 미션을 생성할 때 id가 nil이면 mission.id가 0으로 설정됩니다. 여러 개의 새로운 미션이 동시에 추가될 경우 ID가 중복될 수 있는 심각한 버그입니다. id가 nil일 때는 AutoCounter.getNextID를 사용하여 새로운 ID를 생성해야 합니다.
| mission.id = Int64(id ?? 0) | |
| mission.id = id.map(Int64.init) ?? AutoCounter.getNextID(for: Mission.self, in: context) |
| // func requestNonce(provider: Provider) async throws -> NonceEntity { | ||
| // let nonceRequestDTO = nonceRequestMapper.map(provider.rawValue) | ||
| // let response = try await networkService.request( | ||
| // endPoint: AuthAPI.nonce(dto: nonceRequestDTO), | ||
| // responseType: NonceResponseDTO.self | ||
| // ) | ||
| // return response.toEntity() | ||
| // } | ||
| // | ||
| // func requestLogin(provider: Provider) async throws -> Bool { | ||
| // let nonceEntity = try await requestNonce(provider: provider) | ||
| // let idToken = try await requestIDToken(nonce: nonceEntity.nonce) | ||
| // | ||
| // return try await requestLogin(provider: provider, idToken: idToken) | ||
| // } | ||
| // | ||
| // private func requestLogin(provider: Provider, idToken: String) async throws -> Bool { | ||
| // let requestDTO = loginRequestMapper.map((provider.rawValue, idToken)) | ||
| // let response = try await networkService.request( | ||
| // endPoint: AuthAPI.login(dto: requestDTO), | ||
| // responseType: LoginResponseDTO.self | ||
| // ) | ||
| // saveKeyChain(response: response) | ||
| // saveProvider(provider) | ||
| // | ||
| // let isAgreedTerms = try await isAgreedTerms(accessToken: response.accessToken) | ||
| // let isCompletedJoin = !response.isNewMember && isAgreedTerms | ||
| // return isCompletedJoin | ||
| // } | ||
| // | ||
| // func requestLogin(provider: Provider, idToken: String, name: String?) async throws -> Bool { | ||
| // let isCompletedJoin = try await requestLogin(provider: provider, idToken: idToken) | ||
| // | ||
| // if let name, | ||
| // !name.isBlank, | ||
| // let accessToken = keyChainService.load(key: .accessToken) { | ||
| // try await networkService.request( | ||
| // endPoint: MemberAPI.updateNickname( | ||
| // accessToken: accessToken, | ||
| // dto: .init(nickname: name) | ||
| // ) | ||
| // ) | ||
| // } | ||
| // | ||
| // return isCompletedJoin | ||
| // } | ||
| // | ||
| // func autoLogin() async throws -> Bool { | ||
| // guard let accessToken = keyChainService.load(key: .accessToken) else { | ||
| // return false | ||
| // } | ||
| // | ||
| // guard isTokenExists, | ||
| // try await isAgreedTerms(accessToken: accessToken) else { | ||
| // return false | ||
| // } | ||
| // | ||
| // guard let accessTokenExpirationDate = keyChainService.load(key: .accessTokenExpirationDate), | ||
| // let refreshTokenExpirationDate = keyChainService.load(key: .refreshTokenExpirationDate) else { | ||
| // return false | ||
| // } | ||
| // | ||
| // if tokenValidator.isAccessTokenValid(expirationDate: accessTokenExpirationDate) { | ||
| // return true | ||
| // } | ||
| // | ||
| // if tokenValidator.isRefreshTokenValid(expirationDate: refreshTokenExpirationDate) { | ||
| // do { | ||
| // try await tokenReissuer.reissue() | ||
| // return true | ||
| // } catch { | ||
| // return false | ||
| // } | ||
| // } | ||
| // return false | ||
| // } | ||
| // | ||
| // func getLastLogin() -> Provider? { | ||
| // guard let lastLogin: LastLogin = userDefaultsService.load(key: .lastProvider) else { | ||
| // return nil | ||
| // } | ||
| // return Provider(rawValue: lastLogin.provider) | ||
| // } | ||
| // | ||
| // func logout() async throws { | ||
| // guard let accessToken = keyChainService.load(key: .accessToken) else { | ||
| // BeforeGoingLogger.error(BeforeGoingError.accessTokenMissing) | ||
| // return | ||
| // } | ||
| // try await networkService.request(endPoint: AuthAPI.logout(accessToken: accessToken)) | ||
| // | ||
| // deleteUserInformation() | ||
| // } | ||
| // | ||
| // private func requestIDToken(nonce: String?) async throws -> String { | ||
| // return try await networkService.requestKakaoIDToken(nonce: nonce) | ||
| // } | ||
| // | ||
| // private func saveKeyChain(response: LoginResponseDTO) { | ||
| // let responseData: [KeyChainKey: String] = [ | ||
| // .accessToken: response.accessToken, | ||
| // .refreshToken: response.refreshToken, | ||
| // .accessTokenExpirationDate: tokenValidator.calculateExpirationDate( | ||
| // expiresIn: response.accessTokenExpiresIn | ||
| // ), | ||
| // .refreshTokenExpirationDate: tokenValidator.calculateExpirationDate( | ||
| // expiresIn: response.refreshTokenExpiresIn | ||
| // ) | ||
| // ] | ||
| // | ||
| // for data in responseData { | ||
| // keyChainService.save(data.value, forKey: data.key) | ||
| // } | ||
| // } | ||
| // | ||
| // private func saveProvider(_ provider: Provider) { | ||
| // let lastLogin = LastLogin(provider: provider.rawValue, timestamp: Date()) | ||
| // | ||
| // let _ = userDefaultsService.save(provider.rawValue, key: .provider) | ||
| // let _ = userDefaultsService.save(lastLogin, key: .lastProvider) | ||
| // } | ||
| // | ||
| // private var isTokenExists: Bool { | ||
| // if let accessToken = keyChainService.load(key: .accessToken), | ||
| // let refreshToken = keyChainService.load(key: .refreshToken), | ||
| // !accessToken.isEmpty, | ||
| // !refreshToken.isEmpty { | ||
| // return true | ||
| // } | ||
| // return false | ||
| // } | ||
| // | ||
| // private func isAgreedTerms(accessToken: String) async throws -> Bool { | ||
| // do { | ||
| // let _ = try await networkService.request( | ||
| // endPoint: TermsAPI.getTerms(accessToken: accessToken), | ||
| // responseType: TermsResponseDTO.self | ||
| // ) | ||
| // return true | ||
| // } catch { | ||
| // return false | ||
| // } | ||
| // } | ||
| // | ||
| // private func deleteUserInformation() { | ||
| // for key in KeyChainKey.allCases { | ||
| // keyChainService.delete(key: key) | ||
| // } | ||
| // let _ = userDefaultsService.delete(key: .provider) | ||
| // } |
There was a problem hiding this comment.
현재 사용되지 않는 많은 양의 코드가 주석 처리되어 있습니다. 향후 다시 사용할 계획이 없다면 가독성을 위해 삭제하는 것이 좋습니다.
| try await context.perform { [weak self] in | ||
| guard let self, | ||
| let member = try getMember() | ||
| else { | ||
| throw BeforeGoingError.memberNotFound | ||
| } | ||
|
|
||
| member.nickname = nickname | ||
|
|
||
| try context.save() | ||
| } |
There was a problem hiding this comment.
context.perform 클로저 내에서 getMember()를 호출하고 있습니다. getMember()는 내부적으로 context.fetch를 호출하는데, 이는 동기 작업이므로 context.perform 블록 내에서 안전하게 실행됩니다. 하지만 getMember()가 throws 키워드를 가지고 있으므로, try를 사용하여 에러를 처리해야 합니다. 현재 코드에서는 try가 올바르게 사용되었지만, getMember 함수 자체는 context의 스레드에서만 호출되어야 한다는 점을 명확히 하기 위해 함수 이름을 getMember(on:) 등으로 변경하고 context를 파라미터로 전달하는 것을 고려해볼 수 있습니다. 이는 강제 사항은 아니지만 코드의 스레드 안정성을 더 명확하게 표현하는 데 도움이 될 수 있습니다.
| func checkMission( | ||
| missionID: Int, | ||
| date: String, | ||
| isChecked: Bool | ||
| ) async throws { |
There was a problem hiding this comment.
checkMission 함수에서 date 파라미터가 사용되지 않고 있습니다. 불필요한 파라미터로 보입니다. 이 파라미터를 제거하고 MissionInterface 프로토콜도 함께 수정하는 것을 고려해 보세요.
|
|
||
| func deleteScenario(scenarioID: Int) async throws { | ||
| try await context.perform { [weak self] in | ||
| guard let self else { return } |
There was a problem hiding this comment.
deleteScenario의 context.perform 클로저에서 self가 nil일 경우 아무 동작 없이 return하고 있습니다. 다른 메서드들처럼 BeforeGoingError.unknownError를 throw하여 일관성을 유지하고 에러 상황을 명확히 알리는 것이 좋을 것 같습니다.
| guard let self else { return } | |
| guard let self else { throw BeforeGoingError.unknownError } |
| if let existingMissions = scenario.missions as? Set<Mission> { | ||
| existingMissions.forEach { self.context.delete($0) } | ||
| } |
There was a problem hiding this comment.
updateMissions 함수에서 기존 미션을 모두 삭제하고 다시 생성하는 방식은 미션의 수가 많아질 경우 비효율적일 수 있습니다. 변경된 미션만 찾아 업데이트하거나, 추가/삭제된 미션을 개별적으로 처리하는 방식으로 리팩토링을 고려해볼 수 있습니다.
1 participant
✅ PR 타입
반영 브랜치
setting/#101-coredata -> dev
✨ 변경 사항
📂 관련 이슈
#101