[iOS][Aiden, BMO] team-07 sideDish 2주차 PR 02 #64
Conversation
| } | ||
|
|
||
| override func viewWillDisappear(_ animated: Bool) { | ||
| self.navigationController?.isNavigationBarHidden = true |
There was a problem hiding this comment.
super.viewWillDisappear 호출이 빠진 이유가 있을까요? 아니면 코드 작성사하다가 실수로 빠진걸까요?
| self.detailScrollView.normalPrice.attributedText = self.detailMenuViewModel.salePrice | ||
| self.detailScrollView.deliveryInfo.text = self.detailMenuViewModel.deliveryInfo | ||
| self.detailScrollView.deliveryFee.text = self.detailMenuViewModel.deliveryFee | ||
| self.detailScrollView.totalPrice.text = self.detailMenuViewModel.normalPrice |
There was a problem hiding this comment.
DetailMenuViewController 외에도 여러 뷰컨트롤러에서 detailScrollView.deliveryFee.text 값을 설정한다고 가정하고 추후 이 곳이 변경되어야 할 때 detailScrollView.deliveryFee.text 가지고 있는 모든 뷰컨트롤러를 찾아서 수정해야 합니다. 결론적으로 수정해야 할 범위가 넓어집니다. 그러나 detailScrollView.setup(detailMenuViewModel) 같이 detailScrollView가 내부 로직을 가지고 있다면 수정해야 할 범위도 detailScrollView 내부 즉, 한 곳만 수정하면 되죠! 이처럼 관련된 로직이 한 곳으로 모여 있다면 수정해야 할 범위도 좁아지고 변경 대상도 명확해집니다. 극단적으로 높은 응집도가 항상 좋은 것은 아니지만 적어도 응집도가 높은 객체가 많을수록 좋은 설계가 될 확률이 높습니다. 한 가지 예로 obj.prop = value 처럼 속성을 가져와서 값을 셋팅해야 하는 경우가 많아진다면 과연 이 곳에서 로직을 가지고 있는게 적당한지 의심해 볼 여지가 있습니다!
| self.detailScrollView.orderButton.setTitle("일시 품절", for: .normal) | ||
| self.detailScrollView.orderButton.setTitleColor(.systemGray2, for: .normal) | ||
| self.detailScrollView.orderButton.backgroundColor = UIColor.systemGray5 | ||
| self.detailScrollView.orderButton.isEnabled = false |
There was a problem hiding this comment.
이 부분도 위와 같은 이유와 DetailMenuViewController 객체 입장으로 생각한다면 detailMenuViewModel.stock 이 없을 때 detailScrollView의 orderButton이 어떻게 변해야 할지 알 필요가 없죠! 그렇기 때문에 orderButton이 바뀌는 로직을 detailScrollView로 숨기게 좋습니다🙂
There was a problem hiding this comment.
추가적으로 configureThumbnailImageSize, configureBadges, configureThumbnailImage 도 뷰컨트롤러에서 가지고 있어야 할지, detailScrollView 가지고 있어야 할지 생각해볼 필요가 있습니다🤔
|
|
||
| session.dataTask(with: request){ (_, response, _) in | ||
| guard let response = response as? HTTPURLResponse else { return } | ||
| if (200 ..< 299) ~= response.statusCode { |
There was a problem hiding this comment.
(200 ..< 299) ~= response.statusCode 조건 잘 활용하셨네요👏
| @@ -34,4 +27,41 @@ class DataTaskManager { | |||
| }.resume() | |||
| } | |||
|
|
|||
| static func sendDetailRequest(url: URL?, completion: @escaping (Result<DetailMenu, Error>) -> Void) { | |||
| guard let url = url else { | |||
| print("The URL is inappropriate.") | |||
There was a problem hiding this comment.
앱 디버깅을 위해서 에러 메세지를 남길 때 print도 사용할 수 있지만 로깅 관련 오픈소스나 Apple 지원하는 OSLog 프레임워크를 활용하여 상세한 로그를 남길 수 있습니다. 시간이 될 때 Unified Logging and Activity Tracing WWDC 영상 보세요🙂
| } | ||
|
|
||
| func setBorderWidth() { | ||
| let gray = UIColor(red: 100/255, green: 100/255, blue: 100/255, alpha: 0.3).cgColor |
There was a problem hiding this comment.
실제 프로젝트 진행하다보면 자주 사용하는 색상이 생깁니다. 그래서 주로 사용하는 색상은 한 곳에서 관리합니다. UIColor extension 할 수 있고 Color Asset으로도 관리할 수 있습니다. 이 부분은 꼭 알고 넘어가세요.
| override func awakeFromNib() { | ||
| super.awakeFromNib() | ||
| setupBadgeShape() | ||
| } | ||
|
|
||
| private func setupBadgeLabelConstraint() { | ||
|
|
||
| override func prepareForReuse() { |
There was a problem hiding this comment.
super.prepareForReuse() 의도적으로 호출을 제외하는 것이 아니라면 가급적 호출하는 코드는 꼭 넣어주세요.
| @@ -42,28 +45,19 @@ class MenuCell: UICollectionViewCell { | |||
| func configure(menu: MenuViewModel) { | |||
| self.titleLabel.text = menu.title | |||
| self.bodyLabel.text = menu.body | |||
| self.currentPriceLabel.text = menu.sPrice | |||
| self.currentPriceLabel.text = "\(menu.sPrice)원" | |||
| self.pastPriceLabel.attributedText = menu.nPrice | |||
| guard let url = URL(string: menu.image) else { return } | |||
There was a problem hiding this comment.
URL(string: menu.image) nil이라면 아래 코드 중에서 setBadge 함수가 호출되지 않아도 괜찮은가요?
|
|
||
| @IBAction func pressedOrderButton(_ sender: UIButton) { | ||
| if self.orderViewModel.isOrderAvailable(stock: self.detailMenuViewModel.stock) { | ||
| APIRequestManager.orderPost(orderCount: OrderMenuRequest(count: self.orderViewModel.orderCount), url: URLManager.detailMenu(categoryId: self.detailMenuViewModel.categoryId, detailHash: self.detailMenuViewModel.detailHash), completion: { result in |
There was a problem hiding this comment.
새로운 코드를 작성할 때 본인이 어떻게 작성했는지 늘 염두하고 일관적으로 작성하는 것이 좋습니다. 그래야 새로운 사람이 오더라도 그 기준을 보고 금방 적응할 수 있습니다. FetchDetailMenuUseCase 작성한 것처럼 이 부분도 새로운 UseCase를 분리할 수 있지 않을까요?
| let viewModelList: [MenuViewModel] = menuList.map() { menu in | ||
| let nPrice = stringToAttributedString(menu.nPrice) | ||
| let viewModel = MenuViewModel(image: menu.image, title: menu.title, body: menu.description, sPrice: menu.sPrice, nPrice: nPrice, badges: menu.badge ?? []) | ||
| let price = menu.normalPrice == nil ? nil : String(menu.normalPrice!) |
There was a problem hiding this comment.
강제 언래핑은 가급적 사용 자제하세요. 값이 잘못 들어온다면 앱 크래시가 발생합니다.
| let price = menu.normalPrice == nil ? nil : String(menu.normalPrice!) | |
| var price: String? | |
| if let normalPrice = menu.normalPrice { | |
| price = String(normalPrice) |
|
현재 DetailMenuViewController에서 주문하기 버튼인 orderButton을 누르면 네트워크를 통해 Request를 날리게 됩니다.
|
작업 내용
상세페이지 구현
메뉴 주문하기 구현
수정사항
Cell이 재사용 될 때, 이미지가 잘못 오버랩 되는 경우를 대비하여 이미지를 초기화 시키는 함수를 따로 만들었었는데,
코멘트에 남겨주신 prepareForReuse가 셀이 재사용 될 때 호출되기에 이곳에 초기화 코드를 작성하였습니다.
본래는 API 요청을 하는 객체에 url이 들어있었는데, URL 객체를 분리하였습니다.
data를 가진 객체의 이름을 구체화하였습니다. Data, Get과 같이 의미가 포괄적인 부분은 계속해서 신경써야겠습니다.
고민과 해결
Network를 ParsingManager.decodeData와 같이 제네릭 타입으로 받을 수 있도록 하려고 했는데 잘 되지 않았습니다.
type에서 에러가 나는 것 같은데 조금 더 공부해보고 할 수 있다면 다음 프로젝트에 적용해볼 생각입니다.
FetchMenuResponseUseCase에서 네트워크를 통해 데이터를 가져오는걸로 구현을 했었는데, 지난 PR의 피드백을 보고 수정을 하려 했습니다.하지만 저희가 처음부터 MVVM을 사용하려고 의도했던 것이 아니라 ViewModel을 통해 CollectionView를 깔끔히 하면 좋을 것 같아서,
개발 도중에 이를 살짝 도입한 것이라 현재 클린아키텍처와 구조에 대해 명확히 인지를 하지 못해 수정하지 못했습니다.
질문거리
DetailMenuViewController에서 주문하기 버튼인orderButton을 누르면 네트워크를 통해 Request를 날리게 됩니다.지난번 PR에서 현재 구조에서는 뷰컨트롤러에서 네트워크 로직을 가지고 있는 것이 적절한지 생각해보면 좋겠다고 하셨는데, 이처럼 뷰를 통해 네트워크를 보내는 것은 괜찮은지, 아니면 이 부분 또한 분리해서 다른 단에서 하는 것이 좋은지 궁금합니다!