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

[#44] 지도 권한 로직 개선 #60

Merged
merged 9 commits into from
Jul 24, 2023
Merged

[#44] 지도 권한 로직 개선 #60

merged 9 commits into from
Jul 24, 2023

Conversation

seungchan2
Copy link
Contributor

@seungchan2 seungchan2 commented Jul 23, 2023

🔍 What is this PR?

기존에는 MapViewController에서 지도 권한 설정, 현재 위치 등에 대한 로직들을 처리하고 있었습니다.
이를 DefaultLocationService 객체를 만들어 지도 권한 설정 및 현재 위치 등 그에 따들을 기능들을 다루고 MapViewModel에서 처리를 할 수 있게 수정하였습니다.

📝 Changes

1. LocationService

LocationService 프로토콜입니다.

protocol LocationService {
    var authorizationStatus: BehaviorRelay<CLAuthorizationStatus> { get set }
    func start()
    func stop()
    func requestAuthorization()
    func observeUpdatedAuthorization() -> Observable<CLAuthorizationStatus>
    func observeUpdatedLocation() -> Observable<[CLLocation]>
}

헤딩 기능의 실체 구현은 DefaultLoactionService에서 구현합니다.
현재 MapViewController에서 GoogleMaps에 관한 UI를 처리하고 있는데 이 부분 또한 MapViewModel에서 위도, 경도값만 받을 수 있게 로직을 개선할 예정입니다.

2. View, ViewController 분리

기존 MapViewController에 있는 MapViewModel.Input의 코드는 다음과 같습니다.

tapMenuButton: navigationView.menuButtonObservable

NavigationView에 있는 menuButton에 액션을 주기 위해 접근제어자를 풀고 직접 접근을 하는 구조입니다.
이에 대한 고민이 많았는데 다음과 같이 menuButtonObservable을 통해 버튼에 대한 액션을 처리할 수 있게 수정했습니다.

 private let menuButtonTapped = PublishSubject<Void>()
    private lazy var menuButton = UIButton(frame: .zero,
                                           primaryAction: UIAction(handler: { [weak self] _ in
        guard let self else { return }
          self.menuButtonTapped.onNext(())
    }))


extension NavigationView {
    public var menuButtonObservable: Observable<Void> {
        return menuButtonTapped.asObservable()
    }
}

📸 Screenshot

생략

☑️ Test Checklist

📮 관련 이슈

@seungchan2 seungchan2 added D-2 Dn룰(2일 안에 리뷰 바람) 😁승짠 내가 바로 김승찬 Fix refactor 리팩토링 작업 labels Jul 23, 2023
@seungchan2 seungchan2 self-assigned this Jul 23, 2023
Copy link
Contributor

@Taehyeon-Kim Taehyeon-Kim left a comment

Choose a reason for hiding this comment

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

Good~

Copy link
Contributor

Choose a reason for hiding this comment

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

p3;
다음 파일들은 이미 프로젝트 상에 추가되어있는 친구들이라 프로젝트 파일에 새로 반영하지 않아도 될 것 같아요.
revert 이용해서 되돌리는 방법도 있겠지만, 당장 크게 문제가 있는 내용은 아니라 다음부터 신경쓰면 될 것 같습니다!

  • GoogleMap.plist
  • Config.xcconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re:
넵 !!! 죄송합니당 !!!

Comment on lines 71 to 75
extension NavigationView {
public var menuButtonObservable: Observable<Void> {
return menuButtonTapped.asObservable()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

p3;
객체에 대한 직접적인 접근을 막고 이벤트만 옵저버블 형태로 빼놓은 것은 좋은 접근인 것 같아요.

위에서 쓰는 방식도 좋지만 어차피 RxSwift를 사용하는거라면 RxSwift Traits를 적절하게 이용하는 것도 좋을 것 같습니다.

// NavigationView

fileprivate let menuButton = UIButton()

// Reactive extension
extension Reactive where Base: NavigationView {
  var menuButtonTapped: ControlEvent<Void> {
    base.menuButton.rx.tap
  }
}

ControlEvent가 Observable처럼 값 관찰만 가능하지만, UI에 특화시키기 위해서 Traits형태로 RxCocoa에 정의해놓았기 때문에 적절하게 잘 이용하면 좋을 것 같습니다. menuButtonTapped 프로퍼티를 추가 정의하지 않고도 tap 이벤트를 ViewController에서 전달받아 사용할 수 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re:
감사합니다 !

}

struct Input {
let viewDidLoad: Signal<Void>
let tapMenuButton: Signal<Void>
let tapMenuButton: Observable<Void>
Copy link
Contributor

Choose a reason for hiding this comment

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

p3;
얘만 Observable인 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re:
은주 코드도 섞여있어서 이 부분 논의하고 반영하겠습니다 !

Copy link
Member

Choose a reason for hiding this comment

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

@seungchan2
ControlEvent 로 수정한 부분 확인했습니다! 더 좋은 방법인 것 같아요👍🏻

Comment on lines 118 to 119
.drive { coordinator in
self.move(at: coordinator)
Copy link
Contributor

Choose a reason for hiding this comment

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

p3;
coordinator보다는 coordinates가 특정 좌표로 이동한다의 의미랑 더 맞을 것 같아요!

Comment on lines +156 to 161
func makeMarker() {
let mapCenter = CLLocationCoordinate2DMake(37.57039821, 126.98914393)
let marker = GMSMarker(position: mapCenter)
marker.icon = FogImage.placeMarker
marker.map = mapView
}
Copy link
Contributor

Choose a reason for hiding this comment

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

p2;
얘는 최초에 Center만 설정해주는 메서드인가요?

Comment on lines +16 to +18
var locationManager: CLLocationManager?
var disposeBag: DisposeBag = DisposeBag()
var authorizationStatus = BehaviorRelay<CLAuthorizationStatus>(value: .notDetermined)
Copy link
Contributor

Choose a reason for hiding this comment

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

p2;
private하게 해줘도 괜찮을 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

p2;
@seungchan2 @jane1choi
Protocol(Interface), Implementaion에 대한 네이밍 부분에 대해서 한 번 이야기해보면 좋을 것 같은데
특히 Implementaion에 대한 부분이요! 통일하면 좋을 것 같아요.

선택지는

  • LocationServiceImpl
  • LocationServiceImplementation
  • DefaultLocationService
  • BaseLocationService

이 정도 있을 것 같아요. 요정도를 제일 많이 사용하는 것 같더라구요!
뭐가 좋을지 이야기해보고 통일해서 사용하면 좋을 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

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

개인적인 의견으로는 3번(그냥 Service로 끝나도록) 혹은 1번(lmpl 줄여서 마지막에 붙도록)이 좋은 것 같아요!

final class MapViewModel: ViewModelType {

var disposeBag = DisposeBag()
var authorizationStatus = PublishSubject<LocationAuthorizationStatus?>()
Copy link
Contributor

Choose a reason for hiding this comment

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

p3;
LocationAuthorizationStatus 옵셔널 타입인 이유가 있나요? 위에서 Status가 3가지 밖에 없다고 정의한 것 같은데, 만약 모르는 상태가 또 있다고 한다면 unknown case 정도로 케이스를 추가하는 것은 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re:
해당 부분 고민해보겠습니다 !!!

@Taehyeon-Kim Taehyeon-Kim self-requested a review July 24, 2023 05:32
@seungchan2 seungchan2 merged commit 0b1a6bc into develop Jul 24, 2023
1 check failed
@seungchan2 seungchan2 deleted the feature/#44 branch July 24, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-2 Dn룰(2일 안에 리뷰 바람) Fix refactor 리팩토링 작업 😁승짠 내가 바로 김승찬
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fix] 맵 지도 권한 설정 파일 수정
3 participants