Skip to content
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

ContactMangerUI [Step3] Jason & Blue #19

Open
wants to merge 58 commits into
base: 1_Blue
Choose a base branch
from

Conversation

JasonLee0223
Copy link

ContackMangerUI [Step3] Jason & Blue

안녕하세요, 제이크! (@jryoun1)
💰TEAM BUJA💰의 Blue & Jason입니다!

이번에 Step3과 Bonus Step이 함께 공개되었는데요.
저희는 Bonus Step 진행하는 것보다 Step3 구현과 기존 코드를 리팩토링 및
이번주 ViVi가 방문해주셔서 API Design GuideLine의 네이밍 부분을 아직까지 신경을 못쓰는 것 같다고 말씀해주셔서
Naming 수정에도 신경 써보기로 결정하였습니다!

리뷰 잘 부탁드립니다! 늘 정성스러운 리뷰 감사합니다!!! 🙇🏻‍♀️ 🙇🏻

🧑🏼‍💻 작업 목록

[ Group Work ]

🚀 Step3

  • 연락처 목록 화면의 우상단 + 버튼을 통해 연락처 추가 화면으로 진입 (Modal)
  • 오토 레이아웃과 스택뷰를 활용한 화면 레이아웃
  • 각 필드에 맞는 키보드 종류를 지정
    • 이름 - ASCII Capable
    • 나이 - Number Pad
    • 연락처 - Phone Pad
  • 새 연락처 추가 화면에서 네비게이션 바의 버튼을 누르는 경우 알림창이 나타나도록 구현
    • 취소 버튼을 누르면 정말 취소할 것인지 묻도록 알림창 구현
    • 저장 버튼을 누르면 입력한 정보가 올바르게 입력되었는지 확인하고 올바르지 않은 경우 해당 정보를,
      올바른 경우 연락처 목록 화면으로 이동한 후 입력한 연락처를 목록에 추가하도록 구현
  • 이름은 사용자가 중간에 띄어쓰기를 하더라도 띄어쓰기를 제거
  • 나이는 숫자로만 입력, 세 자리수 이하
  • 연락처는 중간에 -로 구분, -는 두 개 존재
  • 을 제외하고 숫자는 9자리 이상

💭 학습 키워드

  • Delegate Pattern
  • P.O.P (프로토콜 초기구현)
  • Dependency Injection (추가 학습 필요)
  • Auto Layout priority
  • Reusable Cell
  • Generic (추가 학습 필요)

🤔고민과 해결

  • modalpresent했던 VC를 dismiss 했을 때 viewWillAppear가 호출되지 않아 TableView를 reload 할 수 없었던 문제가
    있었습니다.
    // ContactManagerTableViewController.swift
    
    @IBAction func tappedAddNewContactAction(_ sender: UIBarButtonItem) {
        // ...
        addNewContactVC.modalTransitionStyle = .coverVertical
        addNewContactVC.modalPresentationStyle = .automatic
    
        addNewContactVC.delegate = self
    
        present(addNewContactVC, animated: true)
    }
    저희가 작성한 코드에서는 위와 같이 새로운 연락처를 추가하는 VC를 present하는 Style을 automatic으로 설정하였습니다.
    automatic이 어떤 style을 지정하는지 확인하기 위해 lldb를 통해 modalPresentationStyle의 rawValue를 확인해 보았습니다.

1에 해당하는 Style은 `pageSheet`으로, 배경이 되는(호출한) view가 사라지는 것이 아닌 해당 view 위에 레이어를 추가하는 방식으로 그려지게 된다고 학습하였습니다. 이로 인해 첫번째 VC의 view가 view 계층에서 사라지지 않기 때문에 dismiss 하더라도 view가 새로 appear하지 않으므로 `viewWillAppear()`가 호출되지 않는 것으로 파악하였습니다. view가 전환되면서 TableView의 데이터를 reload하는 코드를 delegate가 실행하는 `sendData(newData:)` 내부에 구현하는 것으로 해결하였습니다! > ❓🤔 Jake! 혹시 해당 방법 외에 추천해주실 방법이나, 현업에서 사용하고 계신 방식을 찾을 수 있는 학습 키워드가 있을까요?

🙋 질문 사항

❓Array의 Index 직접 접근하지 않고 해결하는 방법

  • 데이터를 인덱스로 접근하는 것에 대한 위험성은 인지하고 있으나, 이 부분을 어떻게 대체해야 하는가에 대한 좋은 방안이 생각나지 않아 jake에게 질문드립니다!

    @IBAction func tappedSaveButton(_ sender: UIBarButtonItem) {
        // ...
        if errorMessage.isEmpty {
                let sendingContactData = ContactInformation(name: newContactData[0], age: newContactData[1], phoneNumber: newContactData[2])
            //...
            }
    }

    우선, 저희가 생각해 본 방법은 다음과 같습니다.

    • index를 숫자로 직접 넣는 대신 index 값을 가지고 있는 변수를 선언하여 해당 변수로 접근하는 방법

      저희의 경우 배열 형태가 [이름, 나이, 연락처] 순서대로 지정되어 있어서 0, 1, 2 대신 name, age, phoneNumber로 접근하면 어떨까? 했지만, 해당 방법은 index로 접근하는 방식과 크게 다르지 않다고 생각하였습니다.

    • Array의 .first, .last와 같은 메서드 활용

      해당 방법이 인덱스로 직접 접근하는 방식에 비해 안전하다고 생각하였으나, 중간 요소에 접근할 수 없어서 실제로 사용하기는 어렵겠다고 판단하였습니다!

    ❓🤔 저희가 생각한 방법 중 괜찮은 방법이 있거나, 없다면 jake가 추천해주실만한 방법을 찾을 수 있는 키워드가 있을까요?

❓SOLID의 DIP관련하여 질문드립니다.

  • 리펙토링 과정에서 저희가 생각했던 방법은 외부에서 의존성을 주입할 객체의 인스턴스를 생성 후
    초기화 과정에서 넣어주는 방법으로 진행하려고 했습니다.
// ⛔️
let detector = Detector()
let convertor = Convertor()
let checker = Checker()
let contactManager = ContackManager(detector: detector, convertor: convertor, checker: checker)

위와 같이 작성하였더니 UI Target에서 AddNewContactViewController에서 ContactManager를 구현할 때 다시 생성하여 사용해야하는
상황이 발생하여 어색하다고 느껴져 아래와 같이 구현하여 해결하였습니다.

// ✅ 현재 코드
let detector: Detectable
let convertor: Convertable
let checker: Checkable

init(detector: Detectable = Detector(), convertor: Convertable = Converter(), checker: Checkable = Checker()) {
    self.detector = detector
    self.convertor = convertor
    self.checker = checker
}

위와 같이 하다보니 정확하게 DIP 조건을 충족하는 것이 맞는지 확신이 들지 않았습니다..
❓🤔 저희가 리펙토링한 위 코드처럼 작성하여도 맞게 작성한 것인지 궁금합니다!


💭 부연 설명

1️⃣ ContactInformation 데이터 저장 관련

저희가 구현하는 과정에서 놓친 부분이 있었습니다..🥲
앞서 진행했던 프로젝트에서 Model의 데이터를 저장하는

var contactInformationArray: Set<ContactInformation> = []

위 Collection이 있는데 ContactManagerTableViewController에서

private var contactInfomation = [ContactInformation]()

따로 구현하여 처리하게되었습니다. 좀 더 신경써서 Model에 있는 곳에서 관리했어야 했는데
시간이 좀 부족해서 위와 같이 진행하게 되었습니다

2️⃣ 연락처 '-'(hyphen) 로직 관련

다른 팀들을 참고해보며 format 형식을 만들어 관리해주는 팀도 있고

func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {
        
    }

위 메서드를 통해 처리하기도하며 방법은 여러 가지였습니다.
서칭을 통해 이미 오픈 라이브러리로 처리된 부분도 많아서 하드코딩식으로 진행하게되었습니다.
개인적으로(Jason) 이 부분을 TextFieldDelegate의 메서드를 여러개 사용하여 해결할 수 있는
요구사항이라고 생각이 들어 추후 리펙토링 작업을 진행해보려고 합니다.


💻 실행 화면

calledBlu and others added 30 commits February 7, 2023 10:02
- AutoLayout 적용
- Navigation bar 구현
- Error Message 구현 예정
- 기존에 최소 글자 수 제한이 없어서 아무것도 입력하지 않아도 통과되는 문제를 해결
- 1글자 이상으로 최소 글자수를 제한하도록 변경
JasonLee0223 and others added 21 commits February 8, 2023 17:37
- Strive for Fluent Usage에 의거하여 메서드, 파라미터, 프로퍼티 네이밍 변경
- API 설계 지침의 Strive for Fluent Usage 항목에 따라 메서드, 파라미터, 프로퍼티 네이밍 변경
- 인스턴스 생성을 외부에서 주입하며 contactManager initializer 구현
Copy link

@jryoun1 jryoun1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!

이번 스텝도 진행하시면서 열심히 고민하고 이를 코드로 적용해보신 것 같아서 좋아보이네요. 또한 진행하시면서 겪은 문제점들을 pr에 잘 작성해주셔서 답변을 잘 드릴 수 있었네요.

네이밍 관련해서 조금 더 고민해보시면 좋을 것 같아요.
코멘트에 몇 가지 케이스에 대해서 설명남겨드렸습니다.

또한 고민하신 부분들에 대해서도 코드에서 코멘트 남겨드렸어요~ 확인부탁드려요!


import Foundation

internal protocol SystemMenuWorkable {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal로 접근제어를 주신 이유가 궁금합니다 🙋‍♂️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemMenuWorkable 프로토콜 안의 메서드를 모두 private 또는 private(set)으로 감추어서 외부에서 접근하지 못하도록 진행하고
싶었습니다!

⛔️ Cannot find type 'SystemMenuWorkable' in scope

위와 같은 에러가 계속 발생하여 internal로 접근 제어를 주게 되었습니다!


internal protocol SystemMenuWorkable {
func addContact()
func viewContactList(value: Set<ContactInformation>)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view도 괜찮지만 show, display와 같은 동사는 어떤가요?

Copy link
Author

@JasonLee0223 JasonLee0223 Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show 단어가 더 매끄러운 것 같아서 수정하였습니다! 감사합니다! 196f51d

PrintMessage.viewContact(list: value)
}

func exitProgram() -> Bool {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수명만 봤을때는 bool 타입의 반환 값이 있을 것이라고 보이지 않습니다. 개선해볼까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래와 같이 리펙토링 해보았습니다!
해당 커밋 c2eb01a

Comment on lines +10 to +21
protocol InputPossible {
func userInputValue() -> String
}

extension InputPossible {
func userInputValue() -> String {
guard let userInput = readLine() else {
return "F"
}
return userInput
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분을 protocol로 뺀 이유가 궁금합니다 🙋‍♂️

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command Line Tool Target에서 ContactManager가 입력을 받도록 되어있는데 이전 프로젝트에서
프로토콜 초기구현을 적용하고싶었습니다. 당시에는 Blu가 아직 프로토콜에 대해 학습이 되어있지않아서 기회가 되어 적용해보았습니다.

사용한 이유로 ContactManager안에 코드로 구현해도 되지만 로직이 너무 많이 구현되어있어 분리하여 사용하고싶었습니다!

Comment on lines +11 to +12
func checkCorrect(targets: [String]) -> [Bool]
func checkCorrectWord(target: [String]) -> ContactInformation?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 사용하는 부분에서 봤을 때 네이밍을 고려해보면 좀 더 좋을 것 같습니다.

let checkUserInput = contactManager.checker.checkCorrect(targets: newContactData)

위의 사용되는 부분만 보았을 때, checker는 newContactData들이 옳바른지 체크한다. 로 보입니다. 그렇다면 checker는 무엇을 체크하는 걸까요? 실제 함수의 구현부를 보지 않고도 보고 checker가 무엇을 체크하는지 알 수 있을까요?
또한 과연 해당 함수가 무엇을 반환할지 느낌이 오고 있을까요?
만약 bool 배열을 반환할 것이라면 어떠한 네이밍이 오면 좋을까요?

또한 궁금한 점이 있습니다. 2개의 함수에서 인자로 둘 다 문자열 배열을 받는데, 한곳은 targets 다른 곳은 target으로 작성하셨는데 이유가 궁금합니다!
여기서도 반환값이 ContactInformation 타입이 반환되는데 함수명을 고민해보시면 좋을 것 같네요.

Comment on lines 12 to 20
let detector: Detectable
let convertor: Convertable
let checker: Checkable

init(detector: Detectable = Detector(), convertor: Convertable = Converter(), checker: Checkable = Checker()) {
self.detector = detector
self.convertor = convertor
self.checker = checker
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기가 DIP에 관련해서 고민해보신 부분이군요 👍🏻

기존의 방법의 경우에는 각각의 객체들이 구체 타입(Detector(), Checker() 등)에 의존적인 반면에 지금의 구현 방식은 각각의 객체들이 실제 구현이 아닌 인터페이스(프로토콜)를 가지고 있기 때문에 외부에서 해당 인터페이스를 구현한(채택한) 객체들을 init 시점에 넣어주게 된다면 ContactManager는 이제 더 이상 특정 구체 타입에 의존적이지 않게 됩니다.

또한 init 시점에 각 인자들의 기본 값을 적용해주셔서, 필요한 경우에만 다른 객체를 넣도록 구현하신 부분도 잘하신 것 같네요 💯

DIP를 잘 적용해보셨네요 👏👍🏻

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 적용되었다니 다행이네요! ㅎㅎ 추가적인 설명을 통해 DIP를 좀 더 잘 이해할 수 있게 되었네요
자세한 설명 감사합니다! 👍🏻👍🏻👍🏻

Comment on lines +22 to +34
@IBAction func tappedSaveButton(_ sender: UIBarButtonItem) {
let errorMessage = decideErrorMessage()

if errorMessage.isEmpty {
let sendingContactData = ContactInformation(name: newContactData[0], age: newContactData[1], phoneNumber: newContactData[2])

delegate?.sendData(newData: sendingContactData)

dismiss(animated: true)
return
}
successAlert(message: errorMessage)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기가 말씀하신 Index로의 접근 부분이군요.

실제 index로 접근하는 것은 크래시를 발생시킬 수 있는 여지가 있기 때문에 주의가 필요합니다. 해당 부분을 안전하게 접근하기 위해서는 참고 글을 보시면 좋을 것 같네요. 🙏🏻


private func decideErrorMessage() -> String {
var returnMessage = ""
let convertedUserInputData = renderToStringArray()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상수명은 변환된 사용자의 입력 데이터인데, 함수명은 문자열 배열로 그린다 인 것 같습니다. 개선이 필요해보여요!


//MARK: - BarButtonAction
@IBAction func tappedAddNewContactAction(_ sender: UIBarButtonItem) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수명을 좀 더 개선해볼까요?

  • 버튼을 누른 것이므로 아래 정도로 작성해도 충분할 것 같습니다
@IBAction func tappedAddButton(_ sender: UIBarButtonItem)

guard let addNewContactVC = self.storyboard?.instantiateViewController(withIdentifier: "AddNewContactViewController") as? AddNewContactViewController else { return }

addNewContactVC.modalTransitionStyle = .coverVertical
addNewContactVC.modalPresentationStyle = .automatic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modalPresentationStyle에 대해서도 잘 공부해보셨네요. 👍🏻
13이후로 default가 fullScreen에서 automatic으로 변경되었고, 그로 인해서 말씀하신 것처럼 첫번째 vc의 willAppear과 같은 메서드가 호출되지 않게 변경되었습니다.

지금 주어진 가이드에서는 pageSheet으로 뜨는 것이 맞기 때문에 구현하신 것처럼 delegate를 통해서 update를 해주셔도 될 것 같습니다.
그 외에 다른 방법들은 tappedSaveButton 내부에서 dismiss를 할 때 completion으로 첫번째 viewController의 함수를 호출하는 방법도 있을 수 있겠군요.

if let priorVC = self.presentingViewController as? ContactManagerTableViewController {

        self.dismiss(animated: true) {
             priorVC.reloadCollectionView() // reload를 시키는 함수 호출
        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants