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

[iOS][Team10][Dumba, Min] MVVM 패턴적용, DetailView, CoreData #60

Open
wants to merge 18 commits into
base: team10
Choose a base branch
from

Conversation

ghis22130
Copy link
Collaborator

작업 내용

  • 피드백 반영 : 저번 피드백 반영을 하여 수정한 commit 이력이 저번 PR에 쌓여서 몇몇 보이지 않습니다..ㅜ
    • menu: Dictonary -> Double Array
    • Network Service 싱글톤 -> 의존성 주입
    • UseCase 수정: ImageFetch부분에서 UIImageView를 extension 및 Kingfisher Framework 사용
  • CoreData를 이용하여 Network가 안될 시에도 저장되어있는 데이터를 가지고 앱에 뿌려지도록 구현
  • DetailView 구현
  • AlertController를 이용하여 주문가능, 재고부족 등의 Exception Handling 처리

고민 거리 & 개선 방향

  • DetailViewController에 IBOutlet이 다수로 몰려있는 것을 확인했습니다. 이들을 하나의 View안에 넣어서 ViewController를 조금 더 깔끔하게 개선 할 수 있을 것 같습니다.
  • ViewController에서 ViewModel에 해야할 일을 대신 하고 있는 작업(?) 들이 보이는 것 같은데 ViewModel에게 더 위임 할 수 있도록 구조화를 고민 해 보려 합니다.
  • Combine을 사용하는 Publisher property에 private (set) 접근 제한자를 지정해 두었는데 이게 적합한 접근인지 고민입니다.

youngminshim-de and others added 13 commits April 27, 2021 14:10
의존성을 주입해줄 수 있도록 MVVM Pattern 변경 & Dictionary Type이었던 menu property 를 2차원 Array Type으로 변경
반찬디테일 레파지토리, 유즈케이스, 뷰모델, 뷰컨트롤러를 만들어 Flow 연결
BanchanDetailViewModel의 detail을 ViewController와 binding 해주었습니다.
BanchanDetailAPIEndPoint를 생성하여 상세 반찬을 불러올 수 있게 해주었습니다
반찬상세페이지 및 주문하기 버튼 구현
CoreData 관련 CoreDataManager, Moddel, Repository를 생성하였고 DTO, Entity의 자료 변환도 구성하였습니다
BanchanListStorage과 관련된 의존성을 주입하였습니다.
주문하기 버튼 누르면 주문완료 & 재고부족 Alert 창 띄우도록 구현
및 오토레이아웃 버그 수정
재고가 없는 상품일 경우 주문하기 버튼이 일시 품절로 바뀌며 버튼이 비활성화 되어 주문이 불가하도록 설정하였습니다
Copy link

@pay-napster-x pay-napster-x left a comment

Choose a reason for hiding this comment

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

Coordinator가 생겼네요! 😙 MVVM의 단점은 무엇일까요? 왜 Coordinator가 필요할까요?
꼭 알고 쓰셔야합니다.

이번주도 수고 많으셨습니다!!


import UIKit

class AppFlowCoordinator {

Choose a reason for hiding this comment

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

Coordinator 를 도입하셨군요 :) 좋습니다.
혹시 그럼 Coordinator 는 왜 필요할까요?

import UIKit

class AppFlowCoordinator {
private var navigationController: UINavigationController

Choose a reason for hiding this comment

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

navigationController도 let으로 해도 될 것 같습니다.

}

// MARK: - Persistent Storages
private func makeBanchanListStroage() -> CoreDataBanchanListStorage {

Choose a reason for hiding this comment

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

stroage -> storage 오타가 있는 것 같네요 :)


// MARK: - ViewModel
private func makeBanchanDetailViewModel(hash: Int, action: BanchanDetailViewModelAction) -> BanchanDetailViewModel {
return BanchanDetailViewModel(hash: hash, fetchBanchanDetailUseCase: makeFetchBanchanDetailUseCase(), action: action)

Choose a reason for hiding this comment

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

hash 라는 이름이 처음 보는 저에겐 크게 와닿진 않네요.
로직을 파악하고난뒤에 hash가 어떨때 필요한 인자인지 알게 되었습니다.
읽기 좋은 코드는 다른곳을 찾아보지 않고 바로 봤을때 인지 할 수 있는 코드입니다.
매개변수명을 바꿔보는건 어떨까요? 🤔

}
}
} catch {
print("error in sectionEntity")

Choose a reason for hiding this comment

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

PR코드에서는 print는 없어야합니다. :) 이놈의 error처리가 문제내요 항상. 꼭 고민해보세요. 어떻게 해야할까요?

}
try context.save()
} catch {
fatalError()

Choose a reason for hiding this comment

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

여기서는 fatalError가 있네요. 이런식의 에러는 아주 좋지 않습니다. 앱이 죽어버리거든요.

let container = NSPersistentContainer(name: "CoreDataStorage")
container.loadPersistentStores { _, error in
if let error = error as NSError? {
// TODO: - Log to Crashlytics

Choose a reason for hiding this comment

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

Crashlytics에도 로그를 올리게 구현되었나요?
동일합니다. persistentContainer에 load를 실패하면 어떻게 해야할까요?

@IBOutlet weak var totalPriceLabel: UILabel!

@IBOutlet weak var orderButton: UIButton!
private var quantity: Int = 1

Choose a reason for hiding this comment

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

음 이건 어떤 변수인가요?

viewModel.didTappedOrderButton(quantity: quantity) { result, error in
var alert: UIAlertController!

switch result {

Choose a reason for hiding this comment

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

응답에 대한 결과처리를 View에서 하는 역할은 아닌 것 같습니다.

@pay-napster-x
Copy link

Q

Combine을 사용하는 Publisher property에 private (set) 접근 제한자를 지정해 두었는데 이게 적합한 접근인지 고민입니다.

A

해당 속성에 값을 읽어오려면 적합한 접근인게 맞습니다. :)

ghis22130 and others added 5 commits April 30, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-iOS iOS 리뷰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants