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

일기장 [Step3] RedMango, Minsup #141

Open
wants to merge 2 commits into
base: ic_9_redmango
Choose a base branch
from

Conversation

agilestarskim
Copy link

@delmaSong 안녕하세요 delma!!!!
이번 주가 프로젝트 마지막이여서 밤을 새가며 완성을 했습니다...
많이 부족하고 복잡하지만 노력많이했습니다🥲
많은 피드백 부탁드립니다. 감사합니다!!

고민 했던 점

1️⃣ 날씨와 관련된 네트워크 통신만 할게 확실한데 시간이 부족한 상황에서 네트워크 매니저에 확장성이나 유지보수성을 신경써야하는가

  • 요구조건을 보면 날씨와 관련된 API 통신만 할게 분명한데 확장성과 유지보수성을 고려해 코드를 짜야할지 고민되었습니다. 일기라는 특성상 추후에 무언가 다른 API가 추가된다 하더라도 제한적일것만 같고 시간이 부족한 지금의 상황에선 크게 신경 쓸 수 없다고 판단하여 지금과 같이 진행했습니다.

2️⃣ 일기장을 저장하는 순간 네트워크가 불안할 경우 어떤 처리를 해야할지

일기를 저장하는 순간에 네트워크가 불안해서 아이콘 불러오기를 실패하면 어떻게 처리해야할 지 기획을 했습니다.

  • 일기 저장시 아이콘 불러오기 성공 -> png data 통채로 저장
  • 일기 저장시 아이콘 불러오기 실패 -> nil값 저장
  • cell을 랜더링할 때 nil인지 검사
  • nil이 아닐 경우 -> 저장된 이미지 보여줌
  • nil일 경우 -> 해당 일기장이 생성 되었을 시점의 날짜를 기반으로 다시 icon 불러옴 (이전 날씨를 가져오는 API가 필요함)
  • 그것 마저 또 실패하면 빈 이미지 출력

이전에는 날씨아이콘 id를 저장해서 계속 네트워크 통신을 했었지만 지금은 이미지를 통채로 저장해서 네트워크 통신을 계속 하지 않게 끔 기획적으로 변경하였습니다.

이전 날씨를 가져오는 API를 이용해 이미지를 복구하는 작업은 추후에 도전해보겠습니다.

해결하지 못한 점

이미지 크기 조절 실패

정적으로 24값을 주지 않고 옆에있는 CreatedDateLabel의 높이와 같게 하고 싶었으나.. createdDateLabel이 커져버리는 현상이 생겼습니다.. 어떤 것을 고려해봐야할까요...

private func constraintWeatherIcon() {
    NSLayoutConstraint.activate([
        weatherImage.heightAnchor.constraint(equalToConstant: 24.0),
        weatherImage.widthAnchor.constraint(equalToConstant: 24.0)
    ])
}

@delmaSong delmaSong self-requested a review September 19, 2023 04:40
Copy link
Member

@delmaSong delmaSong left a comment

Choose a reason for hiding this comment

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

셀 내 이미지가 간헐적으로 보였다 안보였다 하는데, 이부분 수정이 필요해보입니다~!

스크린샷 2023-09-20 오전 2 56 22

날씨와 관련된 네트워크 통신만 할게 확실한데 시간이 부족한 상황에서 네트워크 매니저에 확장성이나 유지보수성을 신경써야하는가

지금은 학습을 위한 프로젝트라 비교적 스텝이나 요구사항이 확실하고 제한되어있다고 생각하실 수 있는데요, 실제 회사에서 업무를 하게 되신다면 어떨까요? 요구사항이 절대 바뀌지 않을거라고 확신할 수 있을지 모르겠습니다. 왜 유연한 코드를 작성해야한다고 생각하세요? 왜 SOLID 원칙이 중요하다고 하는걸까요? 왜 객체지향적으로 코드를 짜는게 필요하다고 생각하세요? 레드망고와 민섭의 생각이 궁금합니다.

Comment on lines +10 to +17
struct WeatherDecoder {
static func decode(jsonData: Data) throws -> Weather? {
do {
let decoder = JSONDecoder()
let weatherData = try decoder.decode(WeatherData.self, from: jsonData)

return weatherData.weather.first
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

WeatherData라는 타입에 종속적이지 않게 만들 수도 있지 않을까요?

import Foundation

enum WeatherDecodingError: Error {
case decodeFail
Copy link
Member

Choose a reason for hiding this comment

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

Decoding이 fail하는 게 꼭 Weather라는 타입과 연관 있지는 않을 것 같아요

Comment on lines +8 to +9
struct WeatherData: Decodable {
let weather: [Weather]
Copy link
Member

Choose a reason for hiding this comment

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

Weather의 배열타입이라 변수명은 복수형태가 어떨까요?

Comment on lines +18 to +20
func fetch(_ completion: @escaping (Data?) -> Void) {
locationManager.fetchSingleLocation { location in
Task {
Copy link
Member

Choose a reason for hiding this comment

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

location을 전달받을 때 꼭 클로저로 전달받아야 하나요?

Comment on lines +21 to +28
do {
let weather = try await NetworkManager.fetchCurrentWeather(coordinate: location.coordinate)

let icon = try await NetworkManager.fetchWeatherIcon(icon: weather?.icon)
completion(icon)
} catch {
completion(nil)
}
Copy link
Member

Choose a reason for hiding this comment

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

NetworkManager 내의 fetch~ 메서드들은 throws로 에러를 던지고있는데,
여기선 관련한 처리는 없고 실패하면 그냥 nil을 전달할 뿐이네요.
실패했을 때 어떻게 핸들링하면 좋을까요?

Copy link
Member

Choose a reason for hiding this comment

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

그리고 해당 객체는 WeatherFetcher인데, 정작 날씨 관련 정보를 가지고 오는 건 NetworkManager 내부의 메서드가 하는 것 같아요.
어떤 기준으로 해당 객체들의 책임을 나누셨나요?

Comment on lines +11 to +17
struct WeatherFetcher {
let locationManager: LocationManager

init(locationManager: LocationManager = LocationManager()) {
self.locationManager = locationManager
}

Copy link
Member

Choose a reason for hiding this comment

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

WeatherFetcher 내에서 현재의 경,위도 정보만 필요한거라면
해당 값만 주입받는 방법은 어떨까요?
LocationManager 객체 자체를 들고있기엔 현재 구조체의 기능이나 책임이 조금 모호하게 비대한 느낌이 들어요

Comment on lines +22 to +25
self.locationCompletion = completion

locationManager.requestWhenInUseAuthorization()
locationManager.startUpdatingLocation()
Copy link
Member

Choose a reason for hiding this comment

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

self를 붙이고 안붙이고의 기준이 무엇인가요?

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.

None yet

2 participants