[Team-08][iOS] 숙박인원 입력 뷰 생성, 캘린더 뷰 개선 및 버그 수정 #308
Conversation
- 추가로 빌드 에러가 나는 부분을 수정하였습니다.
[Feature] 인원수 뷰 구현
[fix] Calendar 에서 Cell 관리
There was a problem hiding this comment.
피드백 코멘트 남겨두었으니 확인 부탁드립니다.
더하여 작성해주신 내용에 대해서도 추가적인 피드백 드립니다.
- TableView
- View 관련 코드가 중복되는 건 어쩔 수 없는 면도 있습니다. Cell 단위로 재사용이 가능하다면 중복 코드가 줄 수도 있겠지만, 모양이 조금씩 다르다면 어쩔 수 없겠죠. 너무 부담 갖지 마시길 바랍니다 :)
- 다만 모든 곳에서 TableView나 CollectionView 사용이 필요한지에 대해서는 고민 후 시작하면 좋습니다. 중간에도 코멘트를 달았지만 reuse 측면에서 이점이 없다면 괜히 복잡한 구현을 해야할 수도 있으니까요.
- MVVM
- ViewModel의 사용에 대해서는 혼란스러운 게 당연하다고 생각합니다. MVVM 자체에 대한 설명도 제각각인 경우가 많은 것 같아요ㅎㅎ 그러니 당연히 ViewModel의 쓰임 측면에서도 그렇구요.
- 아키텍처에 대한 이해를 높이기 위해서는 직접 구현해보거나, 다른 사람들의 코드를 보며 많이 생각해보는 것밖엔 없는 것 같아요. (저 같은 경우엔 직접 구현해보는 편이 아무래도 이해에 큰 도움이 됐던 것 같네요)
- 어떤 아키텍처를 쓰는 것이 좋은지에 대해서는, case by case라는 답변밖에는 못 드릴 것 같습니다. 프로젝트에서 해결하고자 하는 문제가 무엇일까요? 그리고 미래의 문제를 해결하는 데(유지 보수)에까지 도움이 되려면 어떤 구조로 코드를 짜는 게 좋을까요? 단순히 MVVM이며 MVC며 네이밍이 중요한 게 아니라고 생각합니다. 프로젝트에서의 문제 요점을 찾고 그 문제 해결에 적합한 아키텍처를 적용하는 게 좋은 것 같아요 :)
- 말씀주신대로 사용자의 input을 끊임없이 받아와야하는 프로젝트라면 아무래도 값을 업데이트하기 간편한 구조를 채택하는 게 좋겠죠!
- 어떤 아키텍처를 적용하는 게 좋을지는 현업에서도 늘 어렵게 생각하고 늘 고민하는 부분이니 빠른 시간 내에 빠르게 숙달하려는 부담은 가지지 않으셔도 될 것 같아요. 고민을 해봤다는 사실 자체가 중요하다고 생각합니다!
말이 길어졌네요..ㅎㅎ 마지막까지 정말 고생 많으셨습니다.
다양한 기능 구현하시느라 많이 힘드셨을 것 같아요 :) 부디 주말 푹 잘 쉬셨길 바라고, 다음 프로젝트도 화이팅입니다!
| } | ||
|
|
||
| extension CalendarViewController { | ||
| private func setUpToolBarItems(isEmpty: Bool) -> [UIBarButtonItem] { |
There was a problem hiding this comment.
이 메소드에서는 VC에 ToolBarItem을 set하는 동작까지 수행하는 게 아니라 만들어서 반환만 해주고 있기 때문에
setUp~라고 네이밍을 하면 다소 헷갈릴 수 있습니다 :)
메소드에서 수행하는 동작에 걸맞은 네이밍을 할 수 있도록 항상 신경써주세요!
| let spacing = UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: self, action: nil) | ||
| let skipButton = UIBarButtonItem(title: "건너뛰기", style: .plain, target: self, action: #selector(pushNextVC)) | ||
| let nextButton = UIBarButtonItem(title: "다음", style: .plain, target: self, action: #selector(pushNextVC)) | ||
| let clearButton = UIBarButtonItem(title: "지우기", style: .plain, target: self, action: #selector(clearReservationField)) |
There was a problem hiding this comment.
그리고 이 버튼들을 매번 만들 필요는 없어 보이네요ㅎㅎ
이런 경우에는 프로퍼티로 선언을 해두고, 때에 따라 필요한 버튼만 빼서 set하는 편이 나을 수 있습니다.
| guard | ||
| let cell = collectionView.cellForItem(at: indexPath) as? CalendarViewCell, | ||
| cell.day?.isWithInDisplayedMonth == true, | ||
| cell.day?.isBeforeToday == false else { return } |
There was a problem hiding this comment.
프로젝트 전반적으로 코드 컨벤션이 제각각인 경우가 많아 보입니다!
이런 guard문의 경우에도 팀 내에서 어떤 형식으로 작성을 할지 맞춘다면 보기도 편하고, 코드 작성 시에 어떤 모습으로 작성하면 좋을지 고민도 줄 것입니다.
다음 프로젝트에서는 조금 더 신경 써주세요!
| @@ -37,30 +37,19 @@ class WeekdayView: UIView { | |||
| return label | |||
| }() | |||
|
|
|||
| weekdayLabel.text = getWeekdayText(for: weekdayNumber) | |||
| weekdayLabel.text = weekday.rawValue | |||
There was a problem hiding this comment.
rawValue 값은 enum을 decode하거나 init할 시에 쓰일 수도 있는 값이라 string 표시 용으로는 지양하는 것이 좋다고 생각합니다 :)
웬만하면 rawValue를 직접 쓰기보다는 별개의 var를 선언하면 좋겠어요.
| if indexPath.row == 0 { | ||
| titleText = "성인" | ||
| descriptionText = "만 13세 이상" | ||
| headCount = headCountModel.currentHeadCount.adults | ||
| headType = .adult | ||
| } else if indexPath.row == 1 { | ||
| titleText = "어린이" | ||
| descriptionText = "만 2-12세" | ||
| headCount = headCountModel.currentHeadCount.children | ||
| headType = .child | ||
| } else if indexPath.row == 2 { | ||
| titleText = "유아" | ||
| descriptionText = "만 2세 미만" | ||
| headCount = headCountModel.currentHeadCount.infants | ||
| headType = .infant |
There was a problem hiding this comment.
titleText와 descriptionText같은 경우엔 headType enum 안에서 선언되면 더 깔끔할 것 같네요 :)
| enum HeadCountType: Int { | ||
| case adult = 0 | ||
| case child = 1 | ||
| case infant = 2 | ||
| } |
There was a problem hiding this comment.
여기서의 rawValue는 row 값으로 init할 때 쓰일 수 있겠네요!
There was a problem hiding this comment.
다만 애초에 TableView로 생성해야할지에 대해서는 조금 의문이 듭니다.
셀이 단 3개이고, 사실상 재사용되는 경우도 없다면 간편하게 StackView로 작성하는 것이 더 나을 수 있습니다.
TableView 구현 시엔 기본적으로 구현해야하는 사항이 많고, 셀 재사용이 일어나지 않는다면 성능 상으로도 이득이 없을 것 같아서요.
|
|
||
| class HeadCountCell: UITableViewCell { | ||
|
|
||
| weak var dataSource: HeadCountDataSource? |
There was a problem hiding this comment.
cell이 dataSource를 가지고 있게 된 이유가 있을까요??
| private(set) var adults: Int | ||
| private(set) var children: Int | ||
| private(set) var infants: Int |
There was a problem hiding this comment.
여기서 adults가 0이라면 validate하지 않거나, 무조건 1을 넣어주도록 조건 관리를 하도록 만들어도 괜찮을 것 같네요.
| minusButton.snp.makeConstraints { $0.width.equalTo(30) } | ||
| numberLabel.snp.makeConstraints { $0.width.equalTo(30) } | ||
| plusButton.snp.makeConstraints { $0.width.equalTo(30) } |
There was a problem hiding this comment.
stackView 프로퍼티만으로는 너비 조정이 힘들었을까요?
다 같은 너비라서요ㅎㅎ
| func setTitle(_ title: String) { | ||
| titleLabel.text = title | ||
| } | ||
|
|
||
| func setDescription(_ description: String) { | ||
| descriptionLabel.text = description | ||
| } | ||
|
|
||
| func setNumber(_ number: Int, maxNumber: Int) { | ||
| numberLabel.text = "\(number)" | ||
|
|
||
| minusButton.isEnabled = true; plusButton.isEnabled = true | ||
|
|
||
| if number == 0 { | ||
| minusButton.isEnabled = false | ||
| } else if number == maxNumber { | ||
| plusButton.isEnabled = false | ||
| } | ||
| } |
There was a problem hiding this comment.
이렇게 같은 View에 대한 set 메소드가 많아질 때는,
각 프로퍼티들을 묶어서 하나의 타입화를 할 수 없을지 고민해보면 좋습니다.
이 경우에는 HeadType enum이 될 수 있겠네요.
|
더 이상 수정할 것이 없으시다면 머지해주시면 되겠습니다 :) 화이팅! |
2022/06/10 Pull Request
🔨 관련 Issue
👨🏻💻 작업 내용
🙏🤦🙏 아쉬운 점
현재 검색 조건을 저장하는 뷰 컨트롤러들은 하단의 테이블 뷰를 가지는 뷰 컨트롤러 타입을 상속받아서 하단의 테이블 뷰를 관리할 수 있습니다.
프로젝트 설계시에 "테스트를 고려해서 MVVM 을 하자, 아니면 그냥 MVC를 하자"
크게 저희 프로젝트에서 가장 중요한 부분이 사용자 행동에 반응과 네트워크에 전송이 있었을 것 같은데요.
🙏 To Reviewer
벡: 3주 동안 리뷰 진행해주신 점 감사드립니다! 이번 프로젝트는 너무 많은 부분을 진행하려 한 욕심이 있었나 하는 생각도 듭니다. 개인적으로도 공부를 하면서 진행했다면 좋았을 텐데 피어세션을 진행하다보면 할 얘기가 별로 없는 것이 뭔가 공부가 미흡한 것은 아닌가 하는 생각이 들었습니다.
다마고치: 리뷰에서 많은 인사이트를 주시려고 하셨는데, 아직 제가 받을 준비가 미흡했네요.ㅠㅠ
그래도 이번에 배운 교훈은 코드를 깔끔하게 짜야겠다. 더불어 개발자가 단순히 코드만 만들기 보단 "읽기 쉬운" 코드를 만들어야 한다는 것도 좀 더 와닿았습니다.