-
Notifications
You must be signed in to change notification settings - Fork 36
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
연락처 관리 앱 [Step3] 이지, 카일 #92
base: d_easy
Are you sure you want to change the base?
Conversation
refactor: 불필요 주석 제거
- UpdateContactDelegate 채택 - 기존 AddContactController를 재사용하여 연락처 수정화면 구현
// MARK: - Private Methods | ||
extension ContactListViewController { |
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 extension 으로 하고 안에 있는 메서드들은 private 떼는 방법도 있어요! 참고만 해주세요~
ContactManageViewController(contactManager: contactManager, | ||
contact: contactManager.filteredContact(row: indexPath.row), | ||
navigationBarTitle: "연락처 변경", | ||
delegate: self, | ||
coder: coder) | ||
: ContactManageViewController(contactManager: contactManager, | ||
contact: contactManager.contact(row: indexPath.row), | ||
navigationBarTitle: "연락처 변경", | ||
delegate: self, | ||
coder: coder) |
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 String { | ||
var formattedPhoneNumber: String { |
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.
흠 이거 익스텐션으로 두신 이유가 궁금합니다!
navigationItem.hidesSearchBarWhenScrolling = false | ||
contactSearchController.searchBar.delegate = self | ||
contactSearchController.hidesNavigationBarDuringPresentation = false | ||
contactSearchController.searchBar.placeholder = "이름을 입력해주세요." |
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.
요런 문자열들을 네임스페이스에서 관리하는것은 어떨까요??
creator: { coder in | ||
return ContactManageViewController( | ||
contactManager: self.contactManager, | ||
navigationBarTitle: "새 연락처", |
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.
요런것들도요!
navigationBarTitle: "연락처 변경", | ||
delegate: self, | ||
coder: coder) | ||
: ContactManageViewController(contactManager: contactManager, | ||
contact: contactManager.contact(row: indexPath.row), | ||
navigationBarTitle: "연락처 변경", |
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.
요런 문자열들도요ㅎ
phoneNumberTextField.addTarget(self, | ||
action: #selector(phoneNumberTextFieldEditingChanged), | ||
for: .editingChanged) | ||
guard let contact = contact else { return } |
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 contact = contact else { return } | |
guard let contact else { return } |
@@ -7,70 +7,134 @@ | |||
|
|||
import UIKit | |||
|
|||
final class ContactListViewController: UIViewController { | |||
final class ContactListViewController: UIViewController, UpdateContactDelegate { |
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도 익스텐션르로 채택을 하면 관련 메서드를 한눈에 보기 쉬워서 강추합니다!
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.
수고하셨습니다!!!
이것저것 많이 고민하신 흔적이 느껴졌습니다:)
질문 주신 내용들은 관련 코드에 코멘트 남겼습니다!🙋🏻♂️
ContactManageViewController(contactManager: contactManager, | ||
contact: contactManager.filteredContact(row: indexPath.row), | ||
navigationBarTitle: "연락처 변경", | ||
delegate: self, | ||
coder: coder) | ||
: ContactManageViewController(contactManager: contactManager, | ||
contact: contactManager.contact(row: indexPath.row), |
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.
코멘트를 주신거 보고 이거에 대한 저의 개인적인 의견을 드리자면
분기를 쳐서 ContactManageViewController 를 생성하는 코드가 이렇게 두번씩 있는건 불필요해 보이네요..!
지금
contact: contactManager.filteredContact(row: indexPath.row)
랑
contact: contactManager.contact(row: indexPath.row)
빼고는 다 똑같은데 말이죠..!
그렇다면 그냥 다음과 같이 할 수 있을것 같네요!
let contact = isFiltered ? contactManager.filteredContact(row: indexPath.row) : contactManager.contact(row: indexPath.row)
return ContactManageViewController(인자, contact: contact, 인자, 인자..)
그리고 이렇게 리팩토링을 하면 또 한가지 개선점이 보이는데요..!
filteredContact(row: Int)
와 contact(row: Int)
를 굳이 따로 둘 필요가 없어보이지 않나요?!
하나의 메서드로 합칠 수도 있을것 같네요!
let contact = contactManager.contact(for: indexPath.row, when: isFiltered)
return ContactManageViewController(인자, contact: contact, 인자, 인자..)
질문 주셨던 다음과 같은 상황도 마찬가지로 contactManager 가 알아서 index랑 cell 을 주도록 리팩토링 할 수 있을것 같네요..!
func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
return isFiltered ? contactManager.filteredContactsCount : contactManager.contactsCount
}
...
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
/*{code}*/
let contact = isFiltered ?
contactManager.filteredContact(row: indexPath.row)
: contactManager.contact(row: indexPath.row)
cell.setUpCell(with: contact)
return cell
}
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.
그리고 또 한가지 의문점이 생긴것은,
ContactManageViewController 가 지금 ContactManager를 의존하고 있는데,
생성시에 contact 를 꺼내서 따로 집어 넣는 이유가 있나요???🤔
nameTextField.keyboardType = .default | ||
ageTextField.keyboardType = .numberPad | ||
phoneNumberTextField.keyboardType = .phonePad |
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.
코멘트로 질문 주신 내용이 요기 같아서 답변을 드리자면🤔
요구사항에 따라 다르겠지만 이런식으로 제공하는 키보드를 강제하는 방법도 하나의 방법 같긴 합니다..!
그런데 말씀해주신것 처럼 보통은 사용자 입력을 받을때 validation 로직이 붙는 경우가 많아서 그걸 이용하여 사용자에게 올바른 입력을 유도 하는 경우가 많은것 같아요..!
@junbangg
안녕하세요, 알라딘!
STEP3 PR
보내드립니다..!많이 늦어졌지만 마무리 지어 보았습니다. 시간되실때 확인 해 주시면 감사드리겠습니다! 🙏
🧐 고민했던점
ContactManageViewController
의 재사용연락처 추가화면
과연락처 수정화면
의 인터페이스가 동일하여 기존AddContactViewController
를ContactManageViewController
로 변경하여 재사용하였습니다.navigationTitle
이 생성자를 통해 화면전환 시 주입되어 변경될 수 있게 구현했습니다.reloadData()
VSinsertRows()
row
를 추가하는 작업에 전체tableView
를reload()
하는 것은 비효율적이라고 판단했습니다.append
되는 로직이기 때문에insertRows()
의indexPath.row
를연락처의 갯수 -1
만큼으로 지정하여tableView
에 추가할 수 있게 구현했습니다.🤔 조언을 얻고 싶은 부분
연락처 검색 시 필터된 값을 어떻게 보여줄 것인가
isFiltered
라는Boolean
타입을 통해 기존 데이터를 보여줄 지, 필터된 데이터를 보여줄 지 분기합니다.contacts
는 실제 데이터 저장소,filteredContacts
는contacts
에서 값을 받아와 유저에게 보여지는 임시 저장소 개념으로 생각하여 아래와 같이 구현해 보았습니다.filteredContacts
가 호출되는곳이 많아지면서 위치에따라 네이밍이 적절하지 못하게 느껴지기도 합니다.ex) 초기화면에서부터
filteredContacts
인 것이 부적절알라딘 이라면 어떤 생각이실지 궁금합니다!
textfield의 입력을 제한하는 방법
UISearchBarDelegate
의textDidChange
메서드를 활용해 텍스트필드에 예외적인 문자가 유지되지 않도록 하는 방법을 생각해보았습니다.