-
Notifications
You must be signed in to change notification settings - Fork 79
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
[team11][iOS] - 서버 통신 구현 마무리 및 위치 정보 관련 수정 #305
base: team11
Are you sure you want to change the base?
Conversation
…sxor1993/airbnb_team11 into feat/AccomodationsDataManager
…tailPageVC 이동 로직 수정
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~👍
switch manager.authorizationStatus { | ||
case .authorizedAlways, .authorizedWhenInUse: | ||
manager.startUpdatingLocation() | ||
case .denied: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처리해야 할 로직이 없다면 case .denied 제거해도 될거 같아요!
func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) { | ||
guard let location = locations.first, let homeVC = self.homeVC, homeVC.nowLocation != location else { return } | ||
|
||
NotificationCenter.default.post(name: NSNotification.Name("location"), object: self, userInfo: ["location": location]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotificationCenter 여러 곳에서 사용하기 때문에 하드코딩보다 Notification.Name 확장해서 사용하는 것은 어떨까요?
extension Notification.Name {
static let location = Notification.Name(rawValue: "location")
}
NotificationCenter.default.post(name: .location, object: self, userInfo: ["location": location])
|
||
import Foundation | ||
|
||
struct DetailDTO: Codable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO 명시적으로 붙인 이유가 무엇인가요?
case .success(let data): | ||
handler(data) | ||
case .failure(let error): | ||
print(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert 함수 호출할 때 dispatchGroup.enter 하고, fetchImage에서 성공 상황에서 코드가 문제 되지 않지만, 실패하게 되면 dispatchGroup.leave() 호출하지 않기 때문에 100% 크래시 발생하는 코드입니다. 그래서 failure 상황에서도 handler 호출하도록 변경이 필요합니다.
distanceOfDate = Calendar.current.dateComponents([.day], from: option?.dateRange?.lowerBound ?? Date(), to: option?.dateRange?.upperBound ?? Date()).day ?? 0 | ||
} | ||
|
||
cell.configure(imageData: imageData, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수 매개변수가 많아진다면, 목적을 파악하기 어렵고 혼란을 만들 수 있기 때문에 하나로 최대한 줄이는 것은 어떨까요?
코드를 작성할 때 중요한 것은 한 눈에 목적을 파악할 수 있어야 합니다. 함수 매개변수가 많아질 때 명확한 의도가 있어야 합니다.
configure 함수 파라미터 개수가 어떤 목적으로 전달이 되는건지 알기 어렵기 때문에 CellModel 객체로 만들어서 전달해주는건 어떨까요?
var imageDataArray = [Data]() | ||
var hostImage = Data() | ||
|
||
dispatchGroup.enter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고민했던 지점이 비동기 함수를 동기처럼 처리하여 이미지 처리를 하려고 했던 부분이 DispatchGroup 처리 방법도 있지만 Swift 5.5에서 나온 async, await
개선해볼 수 있습니다. 설명해야 할 부분은 많지만, 비동기 함수 실행을 동기처럼 처리할 수 있는 부분이여서 말씀드려요. 꼭 고쳐야 하는 부분이 아니라 시간이 있을 때 개선할 수 있는 작업으로 코멘트 남긴거니 너무 부담가지지 않으셨으면 좋겠어요~
var imageData: Data | ||
|
||
if item.imageData.count > 0 { | ||
imageData = item.imageData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageData = item.imageData | |
imageData = item.imageData.count > 0 ? item.imageData : Data() |
case .firstCase: | ||
case .mainImages: | ||
return data.imageData.count | ||
case .roomTitle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
많은 케이스가 return 1 처리된다면, default: return 1로 1 리턴해야 하는 case 모두 처리할 수 있지 않을까요?
cell.configure(info: data.hostPage.roomInfo, name: data.hostPage.hostName, image: data.hostPage.hostFace, detail: data.hostPage.detailInfo) | ||
let hostName = "호스트: \(data.hostInfo.name)님" | ||
let hostImage = UIImage(data: data.hostInfo.profileImageData) | ||
let detail = "최대인원 \(data.roomDescription.capacity)명 * 침실 \(data.roomDescription.numberOfBedRoom)개 * 침대 \(data.roomDescription.numberOfBed)개 * 욕실 \(data.roomDescription.numberOfBathRoom)개" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CellModel 객체를 만들어 내부에서 가지고 있는 속성으로 처리하는 것이 어떨까요?
안녕하세요 두두. 금주 2차 PR 올립니다.
�API 통신 관련 문제를 먼저 급하게 해결하느라
저번에 말씀주신 리뷰를 거의 반영하지 못 하였습니다ㅠㅠ
상기의 부분, 모쪼록 참조 부탁드리겠습니다.
금일 PR도 잘 부탁드리겠습니다!ㅎㅎ
[team11][iOS] - 서버 통신 구현 마무리 및 위치 정보 관련 수정
💡 Issue
📝 Tasks
Task1
🔹 작업
🔹 고민과 해결
이 문제를 해결하기 위해
DispatchGroup
과SerialQueue
를 사용했고, 이미지 하나를 받으면 해당 이미지 데이터를 SerialQueue에서 배열에 넣어주고, 모든 이미지를 받아온 후에는DispatchGroup.leave()
메소드를 이용해 필요한 Task들이 끝났음을 알렸습니다. 그리고 endQueue를 이용해 모든 Task가 끝났을 때 Entity를 만들어 VC에 넘겨주도록 했습니다.Task2
🔹 작업
🔹 고민과 해결
이와 같은 형태는 올바르지 않은 듯 보여서 버튼 액션이 실행되면 Notification을 전달하여 VC가 적절한 행위를 하도록 이어지는 방식으로 수정.
Task3
🔹 작업
🔹 고민과 해결
이를 해결하기 위해 TabBarVC에 해당 로직들을 옮겨주었고, 적절한 Location 값을 받아오면 HomeVC에 해당 데이터를 넘겨줌과 동시에 Notification을 전달하여 올바른 API 호출을 할 수 있도록 수정.
추가적으로 Location을 지속적으로 update하는데 이때마다 Notification을 전달할 수는 없으므로, HomeVC에 넘겨준 값과 현재 위치 값이 변경되지 않으면 알림을 보내지 않도록 조건을 추가.