박스오피스 앱 [Step3] 이지, 희동 #60
Conversation
mainViewController를 rootView로 가지고 있는 navigationController
completoinHandler 에서 swift cocurrency로
collectionVIew, activityIndicator, refreshContr
There was a problem hiding this comment.
안녕하세요, 이지, 희동!
짧지 않은 시간이 흘렀네요. 비록 모든 스텝을 수행하지 못한 점은 조금 아쉽지만, 그만큼 두 분이 이번 스텝을 수행하면서 좀 더 단단한 기초를 잡았다고 생각하면 좋겠어요. 이번 프로젝트에서의 마지막 PR이 되는 만큼, 아래에 추가 피드백을 좀 더 자세하게 남겨두었어요. 지금 바로 해결하지 못하더라도, 차차 공부해보시면 좋을 것 같습니다. 두 분 다 고생 많으셨어요!!
고민했던 점 피드백
Completion Handler -> Swift Concurrency 전환
- 가독성 측면에서 무척 공감되네요. 그런데 Completion Handler의 경우 에러 핸들링 안정성, 동기화 처리 등의 문제가 있다고 말씀하셨는데, 어떤 부분에서 문제가 있었던 건가요?
DateFormatter와 Calendar의 미스터리
- 고생하셨네요. 시간과 날짜를 다루는 건 저도 무척 복잡하게 느껴져요. 제공해주신 코드도 잘 작동하는 것 같지만, 각 객체의 역할을 더 잘 살려서 좀 더 읽기 쉬운 코드를 작성해볼 수 있을 것 같아요. 가령 아래처럼요. 더 좋은 방법이 있을 수도 있고요.
func yesterday(dateFormat: String) -> String {
let dateFormatter = DateFormatter()
dateFormatter.dateFormat = dateFormat
dateFormatter.timeZone = TimeZone.autoupdatingCurrent
let calendar = Calendar.current
let yesterday = calendar.date(byAdding: .day, value: -1, to: .now)!
let yesterdayString = dateFormatter.string(from: yesterday)
return yesterdayString
}RefreshControl을 뒤로 보내는 방법
- 질문의 의도 자체를 잘 이해하지 못했어요. 현재 구현된 부분에는 문제가 없어보이는데, 좀 더 자세히 설명해주실 수 있나요?
collectionViewCell의 앞쪽, 뒷쪽이 어떤 걸 말씀하시는 건지 잘 이해되지 않아요.
좋았던 부분
- 코드 컨벤션이 중간중간에 바뀌는 부분이 있지만, 전반적으로 코드가 무척 깔끔해서 읽기 좋았어요.
- Swift Concurrency를 학습해서 적용해보신 부분이 좋았어요.
추가 피드백
- 요구사항의 UI와 다르게 구현된 부분이 있어요. 실제로 일하는 건 아니지만, 가급적이면 요구사항 그대로 지키는 걸 권장해요. 만약 연습삼아, 또는 디자인 취향상 변경이 필요하다면 일단은 리뷰어와 먼저 대화해보는 게 좋을 것 같아요. 현업에서는 질문이 있거나 잘 납득되지 않는 부분이 있다면 구현하기 전에 기획자, 디자이너 분들과 대화해봐야 하고요.
- ⭐️ 순위표 쪽 '신작'의 색상은 항상 빨간색이어야 하는데 색상이 파란색일 때도, 검은색일 때도 있어요.
- 이 문제는 꼭 짚고 넘어갔으면 좋겠어요. 왜 색이 변할까요? 사실 지금 단계에서는 Swift Concurrency보다 이게 더 중요해보여요. '셀 재사용' 키워드로 검색해보시면 됩니다.
- 순위 하락의 경우에도 숫자 표시 양식은 동일해야 하는데 마이너스 표시(
-)가 있어요. - 순위 상승, 하락 모두 숫자의 색은 검정색이어야 하는데 기호와 동일한 색상이 돼요.
- (영화 제목 밑에 있는 레이블에 콜론이나 '명'이 들어간 건 제외할게요.)
- ⭐️ 순위표 쪽 '신작'의 색상은 항상 빨간색이어야 하는데 색상이 파란색일 때도, 검은색일 때도 있어요.
- Completion Hanlder의 단점에 대해 말씀해주시면서 '에러 핸들링 안정성'에 대해 말씀해주셨는데, 프로젝트 전반에서 에러 핸들링이 잘 이루어지고 있는 것 같지 않아요. Throwing functions와 do-catch 구문을 잘 활용해보면 어떨까요?
- 거의 모든 모델 객체가 DTO로 되어 있어요. 이 DTO라는 객체가 서버 응답을 받아 디코딩하기 위한 모델로도 사용되고, 뷰컨트롤러에서 뷰를 그리기 위한 모델로도 사용되고 있어요. 이 모든 역할을 DTO가 수행하는 게 적절할까요?
| extension String { | ||
| func formatNumberString() -> String { | ||
| let formatter = NumberFormatter() | ||
| formatter.numberStyle = .decimal | ||
| guard let number = Double(self), | ||
| let result = formatter.string(from: NSNumber(value: number)) | ||
| else { | ||
| return "" | ||
| } | ||
| return result | ||
| } | ||
| } |
There was a problem hiding this comment.
- 메서드의 네이밍만 보고서는 이 메서드의 역할을 추측할 수 없어요. 더 구체적인 네이밍이 필요해요.
- �self가 숫자형이 될 수 없다면 나머지 로직은 모두 불필요하므로 guard let 바인딩이 앞부분에 나오는 게 좋아보여요.
- NSNumber 같은 타입을 여기서 쓸 필요는 없어 보여요. NumberFormatter의 string(for:) 메서드를 쓰면 될 것 같아요.
- 문제가 있을 경우 ""를 반환하기보다는 차라리 원래의 스트링을 반환하는 게 좋아보여요. 또, 그보다는 String?을 반환하도록 해 실패할 수 있는 메서드라는 걸 알 수 있도록 해주는 게 더 좋아보이고요.
There was a problem hiding this comment.
다음과 같이 수정해 보았습니다!
extension String {
func formatToDecimal() -> String? {
let formatter = NumberFormatter()
guard let number = Double(self),
let result = formatter.string(for: number)
else {
return nil
}
formatter.numberStyle = .decimal
return result
}
}| private let rankChangeLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.translatesAutoresizingMaskIntoConstraints = false | ||
| let attributedString = NSMutableAttributedString(string: "순위변경") |
There was a problem hiding this comment.
영화 데이터를 불러와 셀에 넣어주는 코드를 만들기 전에 테스트로 넣었던 코드 입니다. 실수로 지우지 못했네요..😅
| let label = UILabel() | ||
| label.translatesAutoresizingMaskIntoConstraints = false | ||
| label.font = UIFont.systemFont(ofSize: 16) | ||
| label.numberOfLines = 0 |
There was a problem hiding this comment.
numberOfLines는 텍스트를 생략하지 않고 여러줄에 걸쳐 표시하고 싶을 때 사용하는 속성입니다. 속성값을 0 으로 주면 가능합니다.
저희 앱에서는 별로 필요하지 않은 부분이지만, UILabel의 여러 프로퍼티들을 실험해 보다가 미처 지우지 못한 것 같습니다
| override init(frame: CGRect) { | ||
| super.init(frame: frame) | ||
| configureBorder() | ||
| setupUI() | ||
| } |
There was a problem hiding this comment.
메서드의 네이밍만 봤을 때는 confirgureBorder()가 setupUI() 내부에 들어가는 게 좀 더 자연스러워보이네요.
| private func setupUI() { | ||
| contentView.addSubview(rankStackView) | ||
| contentView.addSubview(movieTitleStackView) | ||
| contentView.addSubview(accessoryButton) | ||
|
|
||
| setupRankStackViewConstraint() | ||
| setupMovieTitleStackViewConstraint() | ||
| setupCellAccessoryButtonConstraint() | ||
| } |
There was a problem hiding this comment.
저는 setupSubviews(), setupConstraints()라는 식의 메서드로 나눠서 관리하는 편인데, 뷰 개수가 그렇게 많지 않으니 이렇게 한 곳에 모아놓는 것도 편리해보이네요.
There was a problem hiding this comment.
민쏜의 방식처럼 한번 바꾸어 볼게요! 그런데 setupUI 를 두 가지의 메서드를 나누는 것과 setupUI 내부에서 두 메서드를 호출하는 형태 중 어느것이 더 좋다고 생각하시나요?
코드를 읽는데 시간이 좀 더 걸리더라도 viewDidLoad 내의 코드를 최대한 줄이고 정리하는 것을 선호 하시나요? 물론 상황에 따라 정답은 없겠지만 이런 사소한 것에도 이유나 기준이 있을지 궁금합니다
| guard let data = data, | ||
| let decodedData = JSONParser().parseJSON(data as! Data, DTO: MovieDetailDTO.self) | ||
| else { | ||
| return | ||
| } | ||
| print(decodedData.movieInfoResult.movieInfo.companys) | ||
| case .failure(let error): | ||
| print(error) | ||
| @objc | ||
| private func loadData() { | ||
| Task { | ||
| let data = try await networkManager.performRequest(with: apiBuilder.buildDailyBoxOfficeAPI(targetDate: APIURLCompnents.QueryValues.targetDate)) | ||
|
|
||
| guard let parsedData = JSONParser().decode(data, DTO: DailyBoxOfficeDTO.self), | ||
| let dailyBoxOfficeData = parsedData.boxOfficeResult.dailyBoxOfficeList | ||
| else { | ||
| return | ||
| } | ||
| dailyBoxOfficeList = dailyBoxOfficeData | ||
| collectionView.reloadData() | ||
| activityIndicator.removeFromSuperview() | ||
| refreshControl.endRefreshing() | ||
| } |
There was a problem hiding this comment.
- 언뜻 봤을 때는 activityIndeicator.removeFromSuperview()를 왜 사용하는지 잘 이해되지 않아요. 아마 데이터가 한 번만 로드되고 나면 다시는 보여줄 필요가 없으니, 여기서 네트워킹이 완료된 후 제거하면 된다고 생각하신 것 같다고 추측할 뿐이에요. 그런데 refreshControl의 action에 loadData가 들어가니, refresh 될 때마다 이 코드가 실행된다고 생각하면 어색하게 느껴져요.
- 제 생각에는 이 메서드의 네트워킹 로직은 별도의 메서드로 분리하고, 맨 처음 데이터를 불러오기 위한 메서드와 새로고침용 메서드를 따로 만들어 네트워킹용 메서드를 각 메서드에서 사용할 것 같아요. 맨 처음 데이터를 불러오기 위한 메서드에서는 시작부분에서 activityIndicator.startAnimating()을 호출하고, 네트워킹용 메서드를 await한 이후에는 activityIndicator.stopAnimating()을 호출하면 더 자연스러울 것 같아요. 그리고 새로고침용 메서드에서는 네트워킹용 메서드를 await한 후에 refreshControl.endRefreshing()을 호출해주고요.
| } | ||
| cell.configureCell(with: dailyBoxOfficeList[indexPath.row]) | ||
| cell.addSubview(MovieListCollectionViewCell()) |
There was a problem hiding this comment.
configureCell 은 cell의 데이터들을 넣어주는 역할을 합니다.
addSubView 는 왜 있는지 모르겠네요 ..ㅎㅎ
| } | ||
|
|
||
| private func setupCollectionView() { | ||
| collectionView.register(UICollectionViewCell.self, forCellWithReuseIdentifier: "cell") |
There was a problem hiding this comment.
register는 컬렉션뷰에 쓰일 cell 을 등록하는 메서드입니다.
를 바로 아래줄에서 사용할 cell 을 identifier 를 통해 등록해주고 있기 때문에 68 line은 지워도 무방합니다. 이전의 코드를 지우지 못한 것 같네요
|
|
||
| class MovieListCollectionViewCell: UICollectionViewCell { | ||
|
|
||
| static let identifier = "MovieListCollectionViewCell" |
There was a problem hiding this comment.
이렇게 정의한 건 무척 좋은데, 아쉽게도 현재 코드에서 사용되지 않고 있네요. 다"MovieListCollectionViewCell"라는 문자열이 프로젝트의 다른 부분에서 그대로 사용되고 있어요. 수정이 필요해보여요.
| struct JSONParser { | ||
|
|
||
| func parseJSON<T: Decodable>(_ movieData: Data, DTO: T.Type) -> T? { | ||
| func decode<T: Decodable>(_ data: Data, DTO: T.Type) -> T? { | ||
| do { | ||
| let decodedData = try JSONDecoder().decode(T.self, from: movieData) | ||
| let decodedData = try JSONDecoder().decode(T.self, from: data) | ||
| return decodedData | ||
| } catch { | ||
| print("파싱 에러") | ||
| print(NetworkError.parseError) | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
에러 처리 방법이 적절해보이지 않아요. 디코딩 과정에서 에러가 발생하면 nil이 반환될텐데, 이렇게 해서는 에러를 정확하게 포착하기 어려울 거예요. 값이 정말로 없어서 nil이 나온 건지, 디코딩 로직의 문제인지도 알 수 없고, 어쩌면 뭔가 문제가 발생했다는 점도 인식하지 못하고 넘어갈 여지도 있어보여요. '에러 전파'라는 키워드로 검색해보시면 좋을 것 같아요.
|
사실 처음 리프레쉬 컨트롤러를 테스트 할때는 리프레쉬 컨트롤러가 돌아갈 때 위 사진처럼 컬렉션뷰를 침범하여 나왔었는데, loadData 메서드로 정리 하는 과정에서 의도한대로 컬렉션 뷰 위에서 한번 걸렸다가 사라지는 것으로 동작하게 되었습니다. (이전에 왜 그랬는지는 정확히 기억나지 않네요 ..🥲) https://bicycleforthemind.tistory.com/39 제가 원했던 것은 이것에 더해 리프레쉬 컨트롤러가 사라질 때도 컬렉션 뷰 뒤쪽으로 가려지면서 사라지는 것 이었습니다. 리프레쉬가 끝남과 동시에 컬렉션뷰가 올라가서 자연스럽게 보이지만, 사실 자세히 보면 여전히 컬렉션 뷰를 침범하면서 사라지는 것을 확인 할 수 있습니다. |
@minsson
안녕하세요 민쏜! 이지, 희동입니다!
박스오피스 STEP3 PR 올려드립니다.
오래 기다리셨습니다.. 리뷰 잘 부탁드려요!! 🙇
✅ 요구사항 & 체크리스트
🙌🏻 PR Point
step3는 컬렉션 뷰를 활용하여 박스오피스 뷰를 구현하는데 중점을 두었습니다.
🤔 고민했던 점
completion handler -> swift concurrency 로 전환
기존의 탈출 클로저와
completion handler를 활용한 네트워킹 코드는 가독성이 떨어져서 흐름을 파악하기가 매우 힘들었습니다.STEP3 에 이르러 기존의 방식을 계속 사용하기에는 너무 어렵게 느껴져서
async / await의 사용법에 대해 알아보았는데, 기존의 방식은 가독성 뿐만 아니라 에러 핸들링 안정성, 동기화 처리 등의 문제가 있다고 알게되어swift concurrency를 도입하기로 했습니다.덕분에 호출 하는 곳에서 네트워킹의 결과를 직접 리턴하여 값을 사용할 수 있는 안정적이고 가독성 좋은 코드가 될 수 있었습니다.
DateFormatter와 Calendar 의 미스터리
어제 날짜를 구해 문자열을 만들기 위한
yesterday메서드를 만들었습니다.사용자의 지역의 시차를 구해 현재 지역 시간을 구하고,
Calendar를 통해 하루 전 날짜를 구했습니다.문제 없이 동작 되는 듯 보이지만, 특정시간 (오후 5~6시 쯤)을 기준으로 하루가 더해지는 현상이 일어납니다.
현재 지역시간인
localicaedDate까지는 정상적으로 확인되지만,Calendar.date를 거치면 하루가 더해지는 것입니다.. 그래서date()의value를 -2 로 바꿔야 어제의 날짜가 나옵니다.문제는
dateFormatter에서 문자열로 변환 할 때의 timeZone이 다른 기준이어서 일어나는 것이었습니다. 위와같이 해결 하면서 UTC와 코드상에서 시간대를 다루는 법에 대해 많은 공부가 되었습니다.refreshControl을 뒤로 보내는 방법?
사소한 점 일수도 있는데,
refreshControl이collectionViewCell의 앞쪽에 위치한것이 어색하게 느껴졌습니다. refreshControl도 UIView를 상속받고 있기 때문에,sendSubviewToBack등의 메서드를 이용해collectionViewCell의 뒤로 보내고 싶었으나 전혀 컨트롤이 되지 않는것으로 보입니다.. view hierachy에도 뜨지 않아서 refreshControl의 위치를 변경하는 방법이 궁금합니다..!이외에도 더 좋은 코드와 구조에 대해 조금씩 고민해 보았지만 컬렉션뷰의 구현을 우선으로 완성 해 보았습니다! 이후에 어떤 방향으로 리팩토링 하면 좋을지 민쏜의 의견이 많은 도움이 될 것 같아요~!