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

[Team-08][iOS] Calendar Model 생성 / CollectionView HeaderCell 추가 작업 #75

Merged
merged 9 commits into from
May 31, 2022

Conversation

SangHwi-Back
Copy link
Collaborator

2022/05/27 Pull Request

🔨 관련 Issue

👨🏻‍💻 작업 내용

  1. 아키텍처 및 클래스 구조 정의
  • 기본적으로는 MVC 패턴을 사용합니다.
  • 하지만, 컨테이너 뷰(UITableView, UICollectionView) 작업이 포함되어야 할 경우 ViewModel 을 추가하여 ViewController 의 역할을 나누게 될 것입니다.
  • 아키텍처, 디자인 패턴에 대한 학습을 진행하였습니다.
  1. 캘린더 전용 Model 생성 및 DTO 생성
  • Swift의 날짜와 관련된 클래스를 학습하였습니다.
    • Date : 날짜 정보를 담고 있음. 수정을 위해서는 DateComponent가 필요합니다. 대부분의 요소들이 Read-Only.
    • DateComponent : Date 혹은 Calendar 클래스를 참고하여 날짜의 특정 요소를 수정할 수 있도록 합니다. 대부분의 요소들이 Read-Write.
    • Calendar : 각 문화권에 따라 날짜를 쉽게(?) 계산할 수 있도록 만든 클래스입니다.
    • 기본적인 흐름 : Calendar 정의 후 DateComponent 추출하여 DateComponent 값 수정이 필요하면 수정한 후에 Date 객체를 추출.
  • Model 이 정확한 날짜를 기준으로 작업을 하였는지, 효율은 괜찮은지 테스트하기 위해 Unit Test 코드를 작성하였습니다.
  1. LocationVC 로 전환 시 SearchController 및 UICollectionView 구현
  • Cell, Header 도 구현

💭 고민한 점과 해결

  1. Repository 를 Model 에 넣을 Dependency Injection 전략을 짜야 합니다. Repository Factory 등을 사용할지 등을 고려해 보았습니다.
  2. Facade Pattern 에 대해 고려해보았지만, ViewModel에 ContinaerView 들의 DataSource/Snapshot 같은 프로토콜 역할이 주어진다면 Facade Pattern 을 사용하는 것은 취소하기로 하였습니다.
  3. SearchVC 에서 LocationVC 로 넘어갈 때 SearchBar 가 작동하지 않는 문제

==> SearchVC 에서 LcationVC 를 띄우는 작업에서 show() 메서드를 사용하고 있었습니다. 그래서 Search Controller 가 작동하지 않는 문제가 있었는데, CollectionView 나 SearchBar의 어떤 설정을 빠뜨린줄 알고 디버깅을 하느라 시간을 많이 허비했었습니다. 그러던 중 그만하고 자야겠다라고 생각이 들었을 때, 다른 분들께 도움 요청을 해서 알아낸 사실이 show() 대신 pushViewController() 를 사용하면 네비게이션 바에 SearchBar가 포함이 된다고 알려주셔서 허무했습니다. 사실 긍정적으로 보면 많은 시간을 써서 문제점을 발견한 것인데, 막상 코드 한줄로 해결이 되니까 허탈한 기분이 들었습니다.

  1. CollectionView Cell 당 크기와 Autolayout Constraints 들이 충돌나서 원하는 UI가 그려지지 않는 문제

==> CollectionView 의 Layout을 그리는 부분은 크게 4부분이 존재합니다.

1. CollectionView 가 있는 VC 의 Layout을 그리는 부분
2. CollectionView 생성시에 들어가는 Flowlayout의 itemSize 메서드
3. UICollectionViewDelegateFlowlay 을 채택하면 사용할 수 있는 sizeForItemAt 파라미터를 받는 메서드 부분
4. CustomCell 의 view layout을 잡는 부분

처음에 작업할 때, 4번의 CustomCell 의 Layout을 잡는 부분에서만 Layout을 짜고선 Layout이 생각대로 되지 않아서 좀 당황했었는데, LocationVC에서 1,2,3번에서 적용한 Constraints 나 Size 정의 때문에 그렇게 되는 것을 알고, 각 요인을 독립 시행을 하면서 어떤식으로 작동하는지 공부했습니다. 그런데 도통 감이 오지 않아 동기 멤버들에게 페어 프로그래밍을 요청해서 해결했습니다. 지금 코드에서는 header 뷰와 Cell 이 공통된 Constraint를 받아야해서, Cell 의 사이즈를 3번 메서드를 통해 정의했고, 전체 CollectionView의 Constraints를 줌으로써 header와 Cell 의 통일성을 가져갔습니다. 4번인 CustomCell 에서 cell 의 구성요소를 제어하는 코드를 만들었습니다.

  1. unit Test measure 함수를 사용해 보았습니다. 여러가지 정보를 반환해주는데 모든 데이터를 해석할 수는 없었습니다. 다음주엔 이 부분에 대해 알아보려 합니다.

📱 데모

[iOS 결과 시뮬레이션]
Simulator Screen Recording - iPhone 13 Pro Max - 2022-05-27 at 13 34 33

[캘린더 모델 기능 테스트 결과]
Calendar_Model_Test_Result

@SangHwi-Back SangHwi-Back added the review-iOS Extra attention is needed label May 27, 2022
@SangHwi-Back SangHwi-Back self-assigned this May 27, 2022
@SangHwi-Back
Copy link
Collaborator Author

안녕하세요 롤로! 저희가 PR 을 보내면서 최종 확인하는 과정에서 추가로 수정할 거리들이 남아있어서 추가로 커밋을 하였습니다. 이번 PR 관련 커밋은 더 이상 없을 예정이오니 참고바랍니다! 리뷰 부탁드리겠습니다~~~

@Damagucci-Juice
Copy link

넵 금요일 PR은 이것으로 더 이상 추가 커밋은 없습니다. ~~ 감사합니다 ~

@eeeesong
Copy link

이번 작업도 고생 많으셨습니다 :)
남긴 코멘트 참고하셔서 머지 후에 다음 작업도 이어서 진행해주세요.
질문 남겨드린 부분들은 답변 해주시면 좋겠습니다!

Comment on lines +19 to +21
var fadeLeft: Bool = false
// 선택되었을 시 오른쪽부터 서서히 채워나가는 이펙트를 줌
var fadeRight: Bool = false

Choose a reason for hiding this comment

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

fadeLeft와 fadeRight가 동시에 true일 경우도 있는 걸까요?👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀 듣고 보니 디자인에는 둘 다 true 일 수 없군요!

fade 라는 것 자체를 enum 타입으로 변경하는 것이 의미 상 맞는 것 같습니다!!


lazy var removeButton: UIBarButtonItem = {
lazy var removeSearchTextField: UIBarButtonItem = {

Choose a reason for hiding this comment

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

이 버튼은 네이밍이 메소드스럽네요ㅎㅎ
그리고 내용을 비우는 거라면 clearSearchField 메소드와 대응되도록 remove보다는 clear가 적절해보입니다 :)

Comment on lines 109 to 111
!searchText.isEmpty
? (navigationItem.rightBarButtonItem = removeButton)
? (navigationItem.rightBarButtonItem = removeSearchTextField)
: (navigationItem.rightBarButtonItem = nil)

Choose a reason for hiding this comment

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

참고로 이 부분은 아래와 같이 쓸 수도 있습니다 :)

navigationItem.rightBarButtonItem = !searchText.isEmpty ? removeSearchTextField : nil

Comment on lines +124 to +125
cell.cityName.text = locations[indexPath.row]
cell.spendingTime.text = locations[indexPath.row]

Choose a reason for hiding this comment

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

locations.count가 곧 셀 개수가 되긴 하지만, index를 통해 정보를 빼 내올 때엔 늘 안전장치를 마련해두는 것이 좋습니다!
존재하지 않는 인덱스를 참조하게 되면 바로 크래시가 나게 되니까요.
그리고 여기서 쓰이는 location들은 추후 다른 정보들을 포함하게 될 가능성도 있어 보이네요. 별개의 타입으로 만들기에 적절해보입니다 :)

Choose a reason for hiding this comment

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

그리고 locations를 뷰 컨트롤러에서 가지고 있을 때의 장단점에 대해서도 생각해보면 좋겠네요ㅎㅎ

Comment on lines +65 to +71
func test_generateDays_performance() throws {
self.measure {
for _ in 1...10000 {
model.getNextMonthDays(from: Date())
}
}
}

Choose a reason for hiding this comment

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

기대한만큼의 성능이 나왔나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

처리되는 속도가 Big-O 표기법으로 표현하자면 O(n) 으로 늘어나는 것을 기대하였고 개인적으로는 그렇게 나왔다고 생각하였습니다.

개인적인 생각보단 구체적인 데이터를 제시해드리고 싶은데 아직 테스트 성능과 관련된 부분에 공부가 부족합니다. 다음 테스트에서는 성능과 관련된 구체적 자료를 제시할 방안이 있는지 찾아보도록 하겠습니다!

Choose a reason for hiding this comment

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

넵넵ㅎㅎ 이렇게 테스트해보신 것 자체가 대단하네요!! 고생 많으셨습니다!

@SangHwi-Back SangHwi-Back merged commit 8b167f8 into codesquad-members-2022:team-08 May 31, 2022
sabgilhun pushed a commit that referenced this pull request Jun 6, 2022
[Android] 두번째주 금요일 PR 리뷰 반영
wooody92 pushed a commit that referenced this pull request Jun 9, 2022
…fo_search

feat: [40] 숙소 예약 내역 상세 조회
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-iOS Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants