-
Notifications
You must be signed in to change notification settings - Fork 3
[Feature] 프로필 설정 UI 및 로직 구현 #79
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
Conversation
eemdeeks
left a comment
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.
고생하셨습니다!!!
persistence까지 혼자 고민해서 해주신 부분이 인상 깊네요 :)
| /// 영구 저장소에 데이터를 저장합니다. | ||
| /// - Parameters: | ||
| /// - data: 저장할 데이터 | ||
| /// - key: 데이터를 저장할 키 값 | ||
| func save<T: Codable>(data: T, forKey key: String) | ||
|
|
||
| /// 영구 저장소에서 데이터를 꺼내옵니다. | ||
| /// - Parameter key: 불러올 데이터의 키 값 | ||
| /// - Returns: 키에 해당하는 데이터를 반환, 데이터가 없거나 불러오지 못하는 경우 `nil` 값 반환 | ||
| func load<T: Codable>(forKey key: String) -> T? |
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.
제네릭으로 Codable한 값을 사용하는 부분이 아름답네요!
| static let adjectives = ["날쌘", "용감한", "귀여운", "활발한", "영리한", "씩씩한", "똑똑한", "빠른", "당찬", "호기심 많은"] | ||
| static let animals = ["여우", "늑대", "토끼", "사자", "다람쥐", "독수리", "곰돌이", "호랑이", "표범", "고양이", "올빼미", "펭귄", "부엉이", "두더지", "물개", "강아지"] |
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.
profileIcons에서는 줄바꿈을 해주셨는데, 여기서는 안해주신 이유를 알 수 있을까요??
제대로 컨벤션을 정하진 않아서 궁금해서 질문합니다!
이부분 컨벤션을 정해보는 것도 좋을 것 같네요!!
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.
profileIcons쪽에서는 ProfileIcon(emoji: "😇", colorHex: "FFE29A") 이런식으로 ProfileIcon을 생성하는 방식이라 확인하기 쉽게 줄바꿈을 해줬는데
adjectives와 animals는 단순 string 값이라 줄바꿈을 안해줬습니다.
근데 사실 길어지다보니 Lint 워닝에 걸리더라고요 ㅜ.ㅜ
컨벤션 정하는거 완전 동의합니다 ~
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.
인정합니다~ 컨벤션 정해봅시다 :)
|
|
||
| protocol ProfileUseCaseInterface { | ||
| /// Repository 프로퍼티 | ||
| var profileRepository: ProfileRepositoryInterface { get } |
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.
해당 유즈케이스를 사용하는 곳에서 repository에 접근이 가능할 것으로 보이는데요,
인터페이스에 추가한 이유가 있을까요?
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.
+
var로 정의 되어 있는 이유가 궁금합니다!!
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.
UseCase는 항상 Repository를 갖고 있어야 생각해서 인터페이스에 추가했습니다.
프로토콜에서 계산 프로퍼티라서 let으로 정의가 되지 않아 var로 했습니다.
| import Foundation | ||
|
|
||
| public final class ProfileUseCase: ProfileUseCaseInterface { | ||
| var profileRepository: ProfileRepositoryInterface |
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.
인터페이스에 정의되어 있다보니, 레포지터리에 외부에서 접근이 가능해 보입니다..ㅜㅜ
해당 유즈케이스를 사용하는 뷰모델에서 레포지터리에 접근해야할 순간이 있을까요??
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.
참 .. 고민이었습니다 ...
외부에서 접근할 일이 없다고 생각하지만 인터페이스에 정의해서 var로 해야 하는 부분이 저도 고민이었습니다. ...
그럼 인터페이스에서 정의하는 것 자체를 지양하는 걸로 하나요 ??
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.
프로토콜의 역할중 구현체가 해당 프로퍼티나 메서드를 갖고있다라는 의미로 해석되는 부분도 있다고 생각합니다.
레포지터리를 갖고있다는 것을 외부에 알려줄 필요가 없다고 생각합니다!!
그렇기에 구현체가 가지고 있어야 할것들을 인터페이스가 정의 해준다라는 것은 역할의 역전이라고 생각이 듭니다..
고로 레퍼지터리는 인터페이스가 갖고있을 필요는 없지 않을까요?.?
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.
저도 딴의 의견에 동의합니다!!
외부에서 어떤 repository를 사용할지는 감추는 건 어떨까요?? 아래 동일한 내용을 리뷰로 남겨 해당 리뷰는 삭제했습니다!
| if let profile: Profile = persistenceService.load(forKey: profileKey) { | ||
| return profile | ||
| } else { | ||
| let randomProfile = Profile(nickname: Profile.randomNickname(), profileIcon: ProfileIcon.profileIcons.randomElement() ?? ProfileIcon.profileIcons[0]) | ||
| saveProfile(profile: randomProfile) | ||
| return randomProfile | ||
| } |
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.
guard let과 if let을 어떻게 구분하고 계신가요??
순수 궁금증입니다!
여기서 왜 if let을 사용했는지!!
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.
개인 취향이지만
저는 guard let은 이후에 옵셔널 푼 값을 계속 사용하고 else문에 간단하게 return 혹은 throw 할 때 사용하고
if let은 해당 값이 옵셔널에 따라 분기처리 할 때 사용합니다.
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.
그렇군요!
확인했습니다 :)
| let decoder = JSONDecoder() | ||
| guard let decoded = try? decoder.decode(T.self, from: data) else { return nil } |
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.
디코딩 오류시에도 nil을 반환 해 주는 군요..!
userDefault에서 가져온 데이터가 디코딩되지 않을 경우는 없겠죠..? 어렵네요..
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.
네엡 !! 일단 디코딩 오류시에도 nil을 반환하도록 했습니다.
흠냐 ... ....... 인코딩 해서 저장했으니까 디코딩이 안 될 확률이 적을 것이라고 생각을 하긴 합니다만. ..... ........
|
|
||
| import Foundation | ||
|
|
||
| let horizontalMargin: CGFloat = 22 |
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.
이런 값들이 많아진다면 나중에 enum이나 싱글톤으로 관리해 주어도 좋을 것 같습니다 :)
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.
동의합니다 ! 빅동 ~
| self.showSelectProfileIconView() | ||
| }, for: .touchUpInside) | ||
|
|
||
| nicknameTextField.delegate = self |
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.
delegate를 생성할 때 클로저 안에서 하는 것과 configureAttribute메서드 내에서 하는 방식 두가지가 섞여있는데, 이를 통일하면 좋을거같아요 😃
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.
네에엡 !!!! 확인 !
| layout.minimumInteritemSpacing = SelectProfileIconLayoutConstant.divider | ||
| layout.minimumLineSpacing = SelectProfileIconLayoutConstant.profileIconLineSpacing | ||
| let collectionView = UICollectionView(frame: .zero, collectionViewLayout: layout) | ||
| collectionView.delegate = self |
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.
여기와 같이 봐주시면 좋겠습니다 ~~
| import Domain | ||
| import Foundation | ||
|
|
||
| final class ProfileViewModel { |
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.
ViewModel프로토콜 채택해주시면 좋을거같습니다 ㅎㅎ
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.
허얼 놓쳤습니다 !!! 확인 ! 감사합니다 ~
taipaise
left a comment
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.
정말 고생하셨습니다! 저희가 이번에 처음 경험하는게 많아서 힘드셨을텐데 잘 하신것 같습니다!!!
| let red = CGFloat((rgb & 0xFF0000) >> 16) / 255.0 | ||
| let green = CGFloat((rgb & 0x00FF00) >> 8) / 255.0 | ||
| let blue = CGFloat(rgb & 0x0000FF) / 255.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.
비트 연산 훌륭합니다 bb
| import UIKit | ||
|
|
||
| final class ProfileIconCollectionViewCell: UICollectionViewCell { | ||
| static let reuseIdentifier = "ProfileIconCollectionViewCell" |
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.
추후 하드 코딩을 줄일 수 있도록 NSObject를 extension 해서 class 이름을 가지고 올 수 있게 해도 좋을 것 같습니다!
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.
이런 방법도 있군요..!! 하나 알아갑니다~
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.
좋습니다 !!! 후추 수정 ~
| profileIconView.addToSuperview(contentView) | ||
| profileIconView | ||
| .center(in: contentView) | ||
| .size(width: profileIconSize, height: profileIconSize) |
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.
단순한 스타일인데요, 저희가 extension한 UIView에서 addToSuperview 또한 Self를 반환하고 있기 때문에 아래와 같이 사용할 수도 있을 것 같습니다! 아니면 혹시 view에 추가하는 것과 레이아웃 잡는 코드를 분리하고 싶으신 걸까요?!!?
| profileIconView.addToSuperview(contentView) | |
| profileIconView | |
| .center(in: contentView) | |
| .size(width: profileIconSize, height: profileIconSize) | |
| profileIconView | |
| .addToSuperview(contentView) | |
| .center(in: contentView) | |
| .size(width: profileIconSize, height: profileIconSize) |
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.
허업 !!
네네 전 기존에 View에 추가하고 Layout을 잡았던 습관때문에 따로 해줬는데 같이 사용할 수 있는 걸 이제 알았습니다 !!
같은 view 프로퍼티에 대해 코드가 줄어드니 한번에 잡는게 더 깔끔한 것 같습니다.
수정하겠습니다 컨벤션 확인 !
| private lazy var iconLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = profileIcon.emoji | ||
| label.font = .systemFont(ofSize: profileIconSize * 0.6) | ||
| return label | ||
| }() |
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.
iconLabel이 VC가 초기화되는 시점에 view에 올라가는 것으로 보입니다.
혹시 lazy var를 사용하는 이유가 있으실까요?
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.
lazy var 사용한 이유는 init시 확정되는 profileIcon, profileIconSize를 iconLabel에서 사용하기 때문이었는데
호오옥시 추후 configureAttribute에서 값을 넣어주는 걸 추천하시나요 ?!?!?!?
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.
ProfileIconView 레이아웃 잡는 코드에 대해서는 같이 적절한 방법을 더 생각해보면 좋을 것 같습니다.
상당히 어려운 부분을 구현해주신거 같은데 정말 고생하셨습니다.!.!
| collectionView.addToSuperview(view) | ||
|
|
||
| collectionView | ||
| .horizontalEdges(equalTo: view, inset: horizontalMargin) | ||
| .verticalEdges(equalTo: view, inset: SelectProfileIconLayoutConstant.profileIconVerticalMargin) |
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.
마찬가지로 view에 add 하는 것과, layout을 잡는 부분을 일부러 분리하신 걸까요?
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.
네 !! 기존 습관이었는데 한방에 하는 걸로 수정해보도록 하겠습니다!!
| (view.bounds.width - 2 * (horizontalMargin) - 2 * (profileIconSpacing)) / SelectProfileIconLayoutConstant.profileIconCountInRow | ||
| }() | ||
|
|
||
| private lazy var collectionView: UICollectionView = { |
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.
초기화 시점이후 profileIconSize를 사용한다면, collectionView도 let으로 사용이 가능할 것 같습니다.
collectionView를 설정하는 코드를 ViewDidLoad 시점으로 옮겨보는 건 어떨까요?
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.
오호 ! 네에 configureAttribute 시점으로 옮기고 let으로 수정해보겠습니다 !
| let updatedProfile = Profile(nickname: updatedText, profileIcon: viewModel.profileSubject.value.profileIcon) | ||
| viewModel.action(input: .updateProfile(profile: updatedProfile)) |
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.
Profile 자체를 VC에서 생성해서 viewModel에 전달해주는 방법도 좋지만, viewModel에 수정된 닉네임을 전달하는 방법은 어떤가요?
예를 들어 viewModel에서 updateNickname같은 Input을 추가하는 방법을 생각해봐도 좋을 것 같습니다.
현재는 프로필을 수정하는 로직을 VC에서 진행하고 있는데, 위와 같은 방식으로 구현하면 ViewModel에서 프로필 수정에 대한 로직을 처리할 수 있을 것 같습니다.
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.
아하 !
저는 updateNickname과 updateProfileIcon 로직이 겹치는 부분이 많다고 생각해서 updateProfile이라는 함수로 같이 사용하도록 했습니다.
딩동이 말한 방식대로 한다면 ViewModel의 profileSubject도 private으로 수정할 수 있을 것 같습니다 ! 확인 !
| let updatedProfile = Profile( | ||
| nickname: "", | ||
| profileIcon: viewModel.profileSubject.value.profileIcon) | ||
| viewModel.action(input: .updateProfile(profile: updatedProfile)) |
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.
요 부분도 위와 같은 방법은 어떠신가요??
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.
예엡 !! 감사합니다 !!
| private lazy var nicknameCountLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.text = "\(viewModel.profileSubject.value.nickname.count)/\(nicknameMaxCount)" | ||
| label.textColor = .gray500 | ||
| label.font = AirplainFont.Body4 | ||
| return label |
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.
작성한 닉네임의 글자 수 설정을 위해 lazy로 선언할 것일까요?
ViewModel의 프로필 퍼블리셔에 binding하여 값이 바뀔때마다 해당 label을 갱신해준다면 let으로 사용이 가능할 것 같습니다!
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.
확인 !! 감사합니다아 ~
- SwiftLint 워닝 수정 (코드의 길이가 길어질 시 줄바꿈) - UseCase 인터페이스에서 repository 제거 - UseCase repository 프로퍼티명 수정 - lazy var 제거 - ViewController에서 ViewModel 수정 로직 제거
taipaise
left a comment
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.
고생하셨습니다~!!!~!!~~
🌁 Background
프로필 설정을 위한 화면들과 Persistence 로직들을 구현하였습니다.
📱 Screenshot
👩💻 Contents
✅ Testing
📝 Review Note
📣 Related Issue
📬 Reference