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

일기장 [STEP 3] Red, 우롱차 #29

Merged
merged 17 commits into from
Jul 4, 2022

Conversation

dnwhd0112
Copy link

@SungPyo
안녕하세요 Weather 😁 😃
이번 스텝도 잘 부탁드립니다.

구현사항 및 고민한 점

네트워킹

Network 객체의 requestData를 만들어 url 에 따라 정보를 가져오도록 설정해 주었습니다.

weatherAPI 사용

OpenWeather 이라는 API를 가져와서 위치에 따른 날씨정보를 가져와 일기에 표시 해주었습니다.

titleView ImageView 수정

navigationController title 에 날씨 이미지를 넣기 위해서
imageView 와 Label 넣은 stackView 를 titleView 에 넣어서 처리 했습니다.

어느 단에서 네트워크를 받을 것인가?

네트워킹을 하는 코드 자체가 어느 시점에 있어야 하는지 고민하였습니다.

이미지를 네트워킹으로 가져올때 익스텐션으로 가져올것인가?

이미지 extension 으로 메서드를 생성해 네트워킹 하는 것이 가장 가독성이 좋다고 생각해서 코드를 그렇게 작성했습니다.

뷰컨트롤러에서 뷰모델에의 네트워킹하는 곳 까지 핸들러를 전달하니 너무 많이 전달하는거 같다.

asyncUpdate 메소드를 통해서 뷰컨 -> 뷰모델 -> 네트워크 까지 전달하는대 사용하기 좀 힘들다.

유스케이스가 현재 레파지토리와 유스케이스가 합쳐논거 같다는 이야기를 들었는대 어떻게 분리하는게 맞는건지 잘 모르겟다.

사실 레파지토리가 뭔지도 잘 모르겠다.
레드는 유스케이스가 뭔지 잘 모르겠다.
우롱차는 아 필요하다고 해서 쓰긴햇는대 설명은 못하겠다.

삽질한점

날씨 정보를 얻기 위해 물어보는게 비동기로 실행이 되어서 처음실행시 위치정보를 얻을 수 없었습니다. 그래서 AppDelegate에서 실행시 날씨정보를 얻도록 변경하였습니다.

let locationManager = CLLocationManager()
        locationManager.requestWhenInUseAuthorization()

import UIKit

extension UIImageView {
func weatherImage(icon: String) {
Copy link

Choose a reason for hiding this comment

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

재사용되면서 생기는 문제가 있는데 오픈마켓때 하셨으니 충분히 수정이 가능하다고 봅니다!

Choose a reason for hiding this comment

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

네!

DispatchQueue.main.async {
      if tableView.indexPath(for: cell) == indexPath {
      cell.setWeatherImage(diary)
        }
}

이런 방식으로 변경 했습니다.

@@ -7,7 +7,7 @@

import UIKit

final class TableViewModel<U: UseCase>: NSObject {
final class TableViewModel<U: CoreDataUseCase>: NSObject {
Copy link

Choose a reason for hiding this comment

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

뷰의 동작(CRUD) 에 대한 실행을 별도의 인터페이스를 통해 ViewModel에 접근하고
Data와 Error의 경우 별도의 파라미터가 아닌 Result 타입으로 변경이 가능할까요? :

Choose a reason for hiding this comment

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

Result 타입으로 수정해 주었습니다.
그런데 Update Delete 와 같은 메서드는 return 타입이 존재하지 않아서
Void 로 넣어 주었는데, Void 로 넣는 것 자체가 Result 타입을 사용할 이유가 있나 하는 생각이 들었습니다.

그런데 CRUD 타입의 통일성을 위해서는 throw 와 Result 타입을 섞어서 쓰는 것 보다는 통일해 주는 것이 가독성 측면에서 좋다고 생각되었습니다.

Void 를 쓰는 로직에 대해서 어떻게 생각하시나요?

Copy link

@SungPyo SungPyo left a comment

Choose a reason for hiding this comment

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

.

@SungPyo SungPyo self-requested a review June 30, 2022 11:26
@SungPyo SungPyo marked this pull request as draft June 30, 2022 11:27
func delete(element: Element) throws
}

final class DiaryUseCase: UseCase {
final class DiaryUseCase: CoreDataUseCase {
Copy link

Choose a reason for hiding this comment

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

이번 PR에 수정된 사항은 아니지만 func requestWeatherData
함수가 WeatherData 하나로 통일되어 있는데, 여러 타입을 받을 수 있도록 수정이 가능할까요?

Choose a reason for hiding this comment

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

제네릭 타입으로 수정하여 Decodable 을 채택한 모든 타입이 들어올 수 있도록 설정 하였습니다

Copy link

@SungPyo SungPyo left a comment

Choose a reason for hiding this comment

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

레드 우롱차 안녕하세요~

지난번 리뷰에 이어서 많은 고민을 하신것 같은데요!

고민했다는것(+ 문제해결을 위한 생각) 자체가 성장에 굉장히 도움이 되는 부분이라고 생각합니다.

비지니스 로직적으로 크게 변경되거나, 문제가 된다고 생각되는 부분은 없기에 코멘트를 통해 몇가지 숙제(?) 를 한번 내봤는데요. 해당 부분 확인 부탁드릴게요 :)

@SungPyo SungPyo marked this pull request as ready for review June 30, 2022 11:31
Monsteel pushed a commit that referenced this pull request Jun 30, 2022
@SungPyo
Copy link

SungPyo commented Jul 4, 2022

뷰모델에 불필요한 부분과 뷰의 동작을 뷰모델로 넘기지 않고 타협(?) 한 부분 등 아쉬운 부분들이 있지만 이정도 흐름이면 만족스럽네요 :)
수고하셨습니다.

@SungPyo SungPyo merged commit 86bdf07 into yagom-academy:ic_5_cherrishred Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants