-
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 [#150] 프로필 사진 변경 API 연동 및 모든 프로필 사진 부분 적용 #152
Conversation
INFOPLIST_KEY_UIApplicationSupportsIndirectInputEvents = YES; | ||
INFOPLIST_KEY_UILaunchStoryboardName = LaunchScreen; | ||
INFOPLIST_KEY_UISupportedInterfaceOrientations = UIInterfaceOrientationPortrait; | ||
INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = "UIInterfaceOrientationLandscapeLeft UIInterfaceOrientationLandscapeRight UIInterfaceOrientationPortrait UIInterfaceOrientationPortraitUpsideDown"; | ||
INFOPLIST_KEY_UIUserInterfaceStyle = Light; | ||
IPHONEOS_DEPLOYMENT_TARGET = 15.0; | ||
LD_RUNPATH_SEARCH_PATHS = ( | ||
"$(inherited)", |
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.
코드 리뷰:
- 코드 변경 내용이 중복되고 있습니다. 두 번의 변경 사항에서 동일한 "INFOPLIST_KEY_NSPhotoLibraryUsageDescription" 및 "INFOPLIST_KEY_UIUserInterfaceStyle" 키가 추가되었습니다.
- 코드 일정부분에서 확신이 없는 듯한 구석이 보입니다. 변경사항이 일상적인 것으로 보이며, 주변 변경 사항과 어울리지 않는 부분이 있습니다.
- 모든 조건에 대한 유효성 검사 및 테스트 케이스가 필요합니다.
개선 제안:
- 변경 내용을 하나로 합치거나 중복된 섹션을 정리하여 코드의 가독성을 향상시킬 수 있습니다.
- 변경 이유를 설명하는 주석을 추가하면 코드의 이해가 용이해집니다.
- 안전하게 테스트된 변경 내용을 포함하는 최상의 방법을 사용하여 변경사항이 앱에 원하지 않는 영향을 미치는 것을 방지할 수 있습니다.
@@ -236,7 +236,8 @@ extension MyPageAccountInfoViewController { | |||
isJoinedApp: true, | |||
isOnboardingFinished: true, | |||
userNickname: loadUserData()?.userNickname ?? "", | |||
memberId: loadUserData()?.memberId ?? 0)) | |||
memberId: loadUserData()?.memberId ?? 0, | |||
userProfileImage: loadUserData()?.userProfileImage ?? StringLiterals.Network.baseImageURL)) | |||
|
|||
OnboardingViewController.pushCount = 0 | |||
} |
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.
전체적으로 코드는 안전해 보이지만 몇 가지 개선점과 위험성이 있습니다:
loadUserData()
호출시 매번 호출되므로, 이것을 변수에 저장하면 성능을 향상시킬 수 있습니다.loadUserData()?.userProfileImage
가 String 타입인데userProfileImage
에 URL을 할당하는 것은 혼란스러울 수 있습니다. 타입 일치를 위해 적절한 처리가 필요합니다.
개선 제안:
loadUserData()
결과를 지역 변수에 저장하여 반복 호출을 방지합니다.userProfileImage
에는 URL 객체를 사용하여 명시적인 형식으로 지정합니다.
위험성:
userProfileImage
가 URL 타입을 가정하지만,String
일 경우 앱이 충돌할 수 있습니다.
코드 리뷰는 주로 안전성과 가독성을 높이기 위해 고려사항을 설명하는 데 중점을 두고 있습니다.
@@ -613,7 +626,8 @@ extension MyPageViewController: DontBePopupDelegate { | |||
isJoinedApp: true, | |||
isOnboardingFinished: true, | |||
userNickname: loadUserData()?.userNickname ?? "", | |||
memberId: loadUserData()?.memberId ?? 0)) | |||
memberId: loadUserData()?.memberId ?? 0, | |||
userProfileImage: loadUserData()?.userProfileImage ?? StringLiterals.Network.baseImageURL)) | |||
|
|||
OnboardingViewController.pushCount = 0 | |||
} else { |
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.
-
DispatchQueue.main.asyncAfter(deadline: .now() + 2.0)
에 대한 주의사항:- 이 코드는 앱이 화면을 로드한 후 2초 후에
refreshData()
함수를 호출합니다. 그러나 앱이 렌더링 되기 이전에 데이터 수신 등의 이슈로 인해 문제가 발생할 수 있습니다.
- 이 코드는 앱이 화면을 로드한 후 2초 후에
-
saveUserData
및loadUserData
메서드 사용에 대한 고려:- 데이터 저장 및 로드되는 시점이 중요하니, 의도와 일관성이 보장되어야 합니다.
-
MyPageViewController
확장부분(methods) 내에서 사용된 변수 및 속성들의 유효성 검증:- 변수 및 프로퍼티 값 변경 전 유효성을 검사하여 예기치 않은 오류가 있는지 확인 필요합니다.
-
클래스 내의 일관된 네이밍 규칙을 따르고 있는지 확인:
- 변수, 함수 및 상수의 명명이 일관되고 이해하기 쉬운지 확인 필요합니다.
-
코드 스타일과 포맷팅:
- 모든 코드 줄이 일관된 스타일과 포맷팅을 따르고 있는지 확인하여 가독성을 향상시키는 데 도움이 되었으면 합니다.
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
우와아앙... 수고하셨습니다 !!!!! ㅜㅜ
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
고생했습니다 👍👻
let is_alarm_allowed: Bool | ||
let member_intro: String | ||
let profile_image: UIImage |
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
카멜 케이스로 바꿔주면 더 좋을 것 같아요!
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
서버 API 명세서에 맞춰놨던거 같아서 주주랑 한번 얘기해보고 수정하겠습니당!
} | ||
|
||
func authSettingOpen() { | ||
let message = "Don't Be 앱에 사진 권한이 없습니다.\n설정으로 이동하여 권한 설정을 해주세요." |
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
StringLiterals로 관리해주면 좋을듯 합니다~
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
넵 수정했습니당
…amDon-tBe/Don-tBe-iOS into feat/#150-profileImageSetting
INFOPLIST_KEY_UIApplicationSupportsIndirectInputEvents = YES; | ||
INFOPLIST_KEY_UILaunchStoryboardName = LaunchScreen; | ||
INFOPLIST_KEY_UISupportedInterfaceOrientations = UIInterfaceOrientationPortrait; | ||
INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = "UIInterfaceOrientationLandscapeLeft UIInterfaceOrientationLandscapeRight UIInterfaceOrientationPortrait UIInterfaceOrientationPortraitUpsideDown"; | ||
INFOPLIST_KEY_UIUserInterfaceStyle = Light; | ||
IPHONEOS_DEPLOYMENT_TARGET = 15.0; | ||
LD_RUNPATH_SEARCH_PATHS = ( | ||
"$(inherited)", |
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.
주석 없이 두 부분이 거의 동일하므로, 코드 리뷰 중 수정이 필요한 부분은 다음과 같습니다:
-
INFOPLIST_KEY_NSPhotoLibraryUsageDescription
와INFOPLIST_KEY_UIUserInterfaceStyle
가 중복 선언되었습니다. 중복 선언을 피하기 위해 이를 최상단에서 한 번만 정의하는 것이 좋습니다. -
문자열 값 "Light"는 따옴표로 감싸져 있지 않으므로, 올바른 문자열로 인식될 수 있도록 따옴표로 묶어주는 것이 좋습니다.
-
주석이 없으므로 이 변경 사항이 왜 필요한지에 대한 설명이 없습니다. 변경의 이유와 의도를 나타내는 주석을 추가하는 것이 좋습니다.
@@ -236,7 +236,8 @@ extension MyPageAccountInfoViewController { | |||
isJoinedApp: true, | |||
isOnboardingFinished: true, | |||
userNickname: loadUserData()?.userNickname ?? "", | |||
memberId: loadUserData()?.memberId ?? 0)) | |||
memberId: loadUserData()?.memberId ?? 0, | |||
userProfileImage: loadUserData()?.userProfileImage ?? StringLiterals.Network.baseImageURL)) | |||
|
|||
OnboardingViewController.pushCount = 0 | |||
} |
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.
이 코드 패치는 상대적으로 안전해 보입니다. 그러나 몇 가지 개선 사항이 있을 수 있습니다:
userProfileImage
에 대한 기본값 또는 유효성 검사가 필요할 수 있습니다.loadUserData()
함수의 반환 형식 및 오류 처리를 다시 검토하여 완전한 데이터 확실성을 보호하는 것이 좋습니다.- 매직 넘버 0을 사용하는 대신
nil
이나 다른 방법을 사용해보세요 좀 더 읽기 쉬운 코드로 변경이 가능합니다.
주의 : 실제 시스템에는 영향을 주지 않고도 이러한 제안을 테스트하려면 테스트 코드를 작성하는 것이 좋습니다.
@@ -613,7 +626,8 @@ extension MyPageViewController: DontBePopupDelegate { | |||
isJoinedApp: true, | |||
isOnboardingFinished: true, | |||
userNickname: loadUserData()?.userNickname ?? "", | |||
memberId: loadUserData()?.memberId ?? 0)) | |||
memberId: loadUserData()?.memberId ?? 0, | |||
userProfileImage: loadUserData()?.userProfileImage ?? StringLiterals.Network.baseImageURL)) | |||
|
|||
OnboardingViewController.pushCount = 0 | |||
} else { |
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.
코드 검토 내용:
- DispatchQueue를 사용하여 데이터 갱신 작업을 2초 뒤에 실행합니다. 이 시간에 대한 명시적인 이유가 없는 한, 왜 즉시 실행되지 않는지 설명이 필요할 수 있습니다.
- saveUserData 함수는 익명 구조체 UserInfo의 생성과 함께 호출됩니다. 이 방법은 코드의 가독성을 저해할 수 있으므로 UserInfo를 먼저 생성하고 나서 해당 인스턴스를 saveUserData에 전달하는 것이 더 좋을 수 있습니다.
- profileEditButtonTapped에서 MyPageEditProfileViewController의 memberId 속성이 직접 설정됩니다. 부모 뷰 컨트롤러에서 자식의 속성을 직접 변경하는 대신, 적절한 초기화 메서드를 통해 값을 전달하도록 구조 변경이 고려될 수 있습니다.
- DontBePopupDelegate 확장에서 userProfileImage를 loadUserData()?.userProfileImage의 반환값으로 설정합니다. null 조치나 오류 처리를 추가하여 nil 값의 가능성을 고려해야 할 것입니다.
개선 제안:
- 불필요한 DispatchQueue 사용을 확인 및 필요한 경우에만 딜레이 목적 설명
- saveUserData 호출 시, UserInfo 인스턴스를 먼저 생성하여 가독성 개선
- 부모에서 자식 객체의 속성을 직접 설정하는 것을 피하고 초기화 메서드를 활용
- userProfileImage의 nil 여부 처리 추가 및 안전성 강화
개선사항을 고려하여 코드를 변경함으로써 가독성, 유지보수성을 향상시킬 수 있습니다.
👻 PULL REQUEST
💻 작업한 내용
💡 참고사항
📸 스크린샷
📟 관련 이슈