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] hoon, karen #140

Open
wants to merge 15 commits into
base: ic_9_karen
Choose a base branch
from

Conversation

Hoon94
Copy link

@Hoon94 Hoon94 commented Sep 14, 2023

STEP 3

안녕하세요. July!(@July911)😊
hoon🐝, keren🦦입니다.

마지막 Step03 PR 보냅니다.

벌써 July께 리뷰 받을 수 있는 3주가 지나갔네요.🥲
July덕분에 많이 배우고 깨달은 것 같습니다. 꼼꼼하고 친절한 리뷰 감사했습니다.

천천히 보시고 리뷰 남겨주시면 감사하겠습니다.



🤔 고민한 부분

1️⃣ 오토레이아웃

  • 일기장을 새로 만든 후에 날씨 아이콘을 받아와서 등록됐을때는 온전하게 오토 레이아웃이 적용됐는데 일기장을 들어갔다 나오면 날씨 아이콘이 커져버리는 문제점이 발생했습니다.

    dateLabel의 높이에 맞추어서 날씨 아이콘의 크기가 변경되도록 제약조건을 잡아주고 dateLabelsetContentHuggingPriorityrequired로 가장 높게 잡아주어 해결했습니다.

     private let dateLabel = {
            let label = UILabel()
            label.font = .preferredFont(forTextStyle: .body)
            label.setContentHuggingPriority(.required, for: .vertical)
            label.setContentHuggingPriority(.defaultHigh, for: .horizontal)
    
            return label
        }()
    
        private let weatherIconImageView = {
            let imageView = UIImageView()
            imageView.contentMode = .scaleAspectFit
    
            return imageView
        }()

2️⃣ API KEY 숨기기

  • API KEY를 감추기 위해 plist 파일을 생성하여 외부에 드러나지 않도록 숨겼습니다. API KEY를 위해 생성한 파일을 git에 저장하고 이후 git에서 추적하지 않도록 설정하여 실제 KEY 값을 사용하였습니다.

    git update-index --skip-worktree Diary/Resource/WeatherInfo.plist


🙇🏻‍♀️ 조언을 얻고 싶은 부분

1️⃣ 날씨 아이콘 오토레이아웃

  • 위에 고민했던 사항 1️⃣에 해당하는 부분으로 이해가 가지 않는 부분이 있어서 July께 조언을 얻고자 합니다. 제약조건을 setContentHuggingPrioritypriority 값을 defaultHigh(750)으로 줄 때와 required(1000)로 줄 때 두 값에 따른 동작 부분이 이해하기 어렵습니다. 750으로 값을 주었을 때 충돌할 만한 조건이 없다고 생각하는데 Cell이 다시 그려질 때 주어진 제약조건과는 다르게 날씨 아이콘이 커지는 문제가 발생하였습니다. defaultHigh보다 큰 값인 751 이상일 때는 문제점이 발생하지 않는데 어떤 부분에서 충돌이 발생했을지 궁금합니다.

2️⃣ 시뮬레이터 CoreLocation 사용

  • 시뮬레이터에서 위치 정보를 받는 방법이 궁금합니다. 시뮬레이터의 Feature > Location > Custom Location을 통해서 직접 위치 좌표를 기입하는 방법이 아닌 GPS 기반으로 실제 테스트하고 있는 장소의 위치 정보를 받을 수 있는 방법이 있을지 궁금합니다. 시뮬레이터에 기본으로 설치되어 있는 지도 앱을 테스트한 경우에도 현재 위치가 아닌 기본 값으로 설정되어 있는 위치 좌표 값을 받아오고 있었습니다.

class CacheManager {
static let shared = NSCache<NSString, UIImage>()

private init() {}
Copy link

@July911 July911 Sep 15, 2023

Choose a reason for hiding this comment

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

1.인터페이스로 아무것도 지원하지 않나요 ? 그렇다면 매니저라는 이름이 맞나요 ?
2. 상속을 하나요 ?

Copy link
Author

Choose a reason for hiding this comment

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

35ac31f

  1. 싱글톤 객체를 사용하지 않고 캐시 작업을 수행하는 DiaryCell에 객체를 생성하여 사용하기에는 View에 불필요한 객체가 들어가는 것 같아 CacheStore로 네이밍을 변경하였습니다.
  2. final 키워드를 사용하였습니다.


import Foundation

protocol URLalbe {
Copy link

Choose a reason for hiding this comment

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

이름이 조금 어색하네요 ... ㅎㅎ
NetWorkManager가 API 형태로 받을수있는 프로토콜인거죠 ?

Choose a reason for hiding this comment

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

5c59781

삭제 해주었습니다.

return nil
}

return "https://api.openweathermap.org/data/2.5/weather?lat=\(latitude)&lon=\(longitude)&appid=\(APIKey)"
Copy link

Choose a reason for hiding this comment

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

URL구성요소로 나눠봐도 좋았을 것 같습니다 ~

Copy link
Author

@Hoon94 Hoon94 Sep 15, 2023

Choose a reason for hiding this comment

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

5c59781

URLComponents를 통해 각각의 구성요소에 맞게 URL을 생성하였습니다.😊


var url: String? {
switch self {
case .weatherData(latitude: let latitude, longitude: let longitude):
Copy link

@July911 July911 Sep 15, 2023

Choose a reason for hiding this comment

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

latitude: let latitude,
맨 앞에 latitude 가 꼭 필요한가요 ?

Choose a reason for hiding this comment

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

5c59781

전달인자 레이블인 latitude, longitude를 생략하였습니다.

guard let file = Bundle.main.path(forResource: "WeatherInfo", ofType: "plist"),
let resource = NSDictionary(contentsOfFile: file),
let key = resource["API_KEY"] as? String else {
fatalError("⛔️ API KEY를 가져오는데 실패하였습니다.")
Copy link

Choose a reason for hiding this comment

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

API key를 못가져오면 크래시가 나나요 .. ?

Copy link
Author

Choose a reason for hiding this comment

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

336190a

toast 메세지를 사용하여 사용자에게 안내하였습니다.🍞

label.setContentHuggingPriority(.defaultHigh, for: .horizontal)

return label
}()

private let weatherIconImageView = {
let imageView = UIImageView()
imageView.contentMode = .scaleAspectFit
Copy link

Choose a reason for hiding this comment

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

.scaleAspectFit 말고 나머지는 뭐가 있고 특징은 무엇인가요

Choose a reason for hiding this comment

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

  • scaleAspectFit : 가로 세로 비율을 유지하면서 뷰의 사이즈에 맞게 이미지를 늘리는 옵션 (비율이 맞지 않을 경우 내부에 여백이 생김)
  • scaleAspectFill : 가로 세로 비율을 유지하면서 뷰의 사이즈에 맞게 이미지를 꽉 채우는 옵션 (이미지의 한 부분이 잘려 보일 수 있음)
  • scaleToFill: 전체 이미지가 다 나올 수 있도록 이미지를 꽉 채우는 옵션 (비율 무시)
  • center: 이미지를 이미지 뷰의 가운데에 배치 (이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음)
  • .top, .bottom, .left, .right: 가로 세로 비율을 유지하면서 이미지를 이미지 뷰의 위, 아래, 왼쪽 또는 오른쪽에 배치(이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음)
  • .topLeft, .topRight, .bottomLeft, .bottomRight: 가로 세로 비율을 유지하면서 이미지를 이미지 뷰의 네 모서리 중 하나에 배치(이미지가 이미지 뷰보다 크면 일부 이미지가 잘릴 수 있음)

@@ -57,10 +66,45 @@ final class DiaryCell: UITableViewCell {
fatalError("init(coder:) has not been implemented")
}

func configureCell(title: String?, date: String, preview: String?) {
override func prepareForReuse() {
weatherIconImageView.image = nil
Copy link

@July911 July911 Sep 15, 2023

Choose a reason for hiding this comment

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

  1. super콜이 필요없나요 ?
  2. 왜 image = nil 이 필요한가요 ?

Choose a reason for hiding this comment

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

7d36fcf

  1. super콜을 수정하였습니다.
  2. imagenil이 필요한 이유는 cell이 재사용 될 때 기존에 날씨 아이콘 이미지가 없던 cell에도 재사용되기 전의 이미지가 들어가는 문제점이 발생해서 초기화를 위해 할당해주었습니다.

return
}

guard let cache = CacheManager.shared.object(forKey: NSString(string: icon)) else {
Copy link

@July911 July911 Sep 15, 2023

Choose a reason for hiding this comment

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

캐시된게 있으면 그걸 쓰고 아니면 불러오라는 뜻 같은데, 흐름상이나 가독성 면에서 if let이 더 좋아보였을 것 같기도 하네요 !
상단에서 빠른 종료가 아니라 있는지 없는지 여부를 체크할떄는 if guard 를 잘 생각해서 쓰면 좋을 것 같습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

6ee190e

if let으로 수정하였습니다.🤗

// Created by hoon, karen on 2023/09/14.
//

enum NetworkError: Error {
Copy link

Choose a reason for hiding this comment

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

LGTM

return
}

let weather = WeatherAPI.weatherData(latitude: latitude, longitude: longitude)
Copy link

@July911 July911 Sep 15, 2023

Choose a reason for hiding this comment

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

weatherData( 는 조금 어색한 것 같습니다.
무엇을 위한 API 인지가 들어가는 이름이 적합해보입니다.

Copy link
Author

Choose a reason for hiding this comment

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

5c59781

localWeather라는 네이밍으로 수정했습니다.😊

@Hoon94 Hoon94 marked this pull request as draft September 15, 2023 09:54
@Hoon94 Hoon94 marked this pull request as ready for review September 15, 2023 19:21
Stpe03 README 수정
README : 블로그 아이콘 이미지 다시 올림
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

3 participants