-
Notifications
You must be signed in to change notification settings - Fork 147
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 2-1] maxhyunm #305
base: ic_9_maxhyunm
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨어요 맥스
저는 뷰는 최대한 쪼개는게 좋아서,
화면 하나라고 뷰컨이 하나일 필요는 없을거 같아요
화면 하나에도 10개의 뷰컨과 뷰모델을 가지고, 그 아이들을 조합한 부모 뷰컨이 있는게 좋다고 생각합니다
case doing = 1 | ||
case done = 2 | ||
|
||
var name: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 아이들은 customStringConvertible같은 프로토콜을 써도 괜찮을거 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomStringConfertible을 전에 활용해 보았을 때, String 타입이 들어가야 하는 경우에는 결국 .description으로 접근해야 했던 기억이 있어서 연산 프로퍼티로 진행했었습니다!(이 부분은 제가 잘못 알고 있는 것이라면 말씀 부탁드립니다 😭)
그런데 string 값으로 관련 status 값을 가져와야 하는 경우가 발생해서, name 대신 rawValue를 사용하는 쪽으로 수정하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네넵 사실 rawValue나 description이나 취향차이고 거기서 거기긴 합니다 ㅎㅎ;
|
||
import CoreData | ||
|
||
class CoreDataManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class로 선언한 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하나의 매니저를 만들면 동일 인스턴스를 여기저기서 공유하게 되기 때문에 class로 생성했습니다! 다만 해당 타입 내부에 있는 프로퍼티가 let 뿐이라서 struct로 생성해도 문제가 없었을 것 같네요... struct로 수정하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 그리고 class는 무조건 final! 습관 들여주세요
ProjectManager/ProjectManager/Model/CoreData/CoreDataManager.swift
Outdated
Show resolved
Hide resolved
ProjectManager/ProjectManager/List/ToDoListViewController.swift
Outdated
Show resolved
Hide resolved
ProjectManager/ProjectManager/List/ViewModel/ToDoListViewModel.swift
Outdated
Show resolved
Hide resolved
self.toDoView.reloadTableView() | ||
self.doingView.reloadTableView() | ||
self.doneView.reloadTableView() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 데이터를 reload하지 않고 사용할 수 있는 방법에 대해 고민하면 좋을 듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list를 status 값에 따라 세 개로 나누어 각각의 리스트에 바인딩해주는 방식으로 수정하겠습니다...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각각 바인딩 하는 방식도 의도한 거긴 한데요,
reload를 부르는 방법이 아닌, performBatchUpdate를 하거나
delete update 같은 방식도 고민해보시면 좋을 듯 합니다.
안녕하세요 하비(@havilog)! 🔹 뷰컨트롤러와 뷰모델 나누기뷰컨트롤러와 뷰모델을 최대한 쪼개는 것이 좋을 것 같다는 말씀을 듣고 고민하다가, 🔹 배치 업데이트테이블뷰에 배치 업데이트를 적용하려면 데이터의 생성/수정/삭제에 따라 각기 다른 작업을 진행해야 하는 것을 확인하였습니다. (예: delete를 진행할 경우 해당 row를 골라 지워주어야 함) 이 부분에 있어 많은 고민을 하였는데... 결과적으로는 create, update, delete의 상태값을 가질 수 있는 Action이라는 타입을 생성하여 자식 뷰모델에서 Observable로 action 프로퍼티를 갖고 있도록 하였습니다. 🔹 기타데이터 추가시 제대로 화면이 바뀌는지 테스트하기 위해 BaseViewController의 + 버튼을 누르면 더미데이터가 생성되도록 임시 코드를 추가해 두었습니다.(#if DEBUG 처리하였고, STEP 2-2 작업시 삭제 예정입니다!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 의도는 하나의 todo를 만들고
여러개의 todo로 확장하면서
어떻게 확장성 있게 처음부터 설계할 수 있을지도 중요했을거 같은데
지금 상태에서 뭔가 요구사항을 더 수행하기보다는
viewmodel을 어떻게 develop할 수 있을지 고민해보시면 좋을 듯해요
func setControllerTitle(title: String) { | ||
self.controllerTitle = title | ||
} | ||
|
||
func setControllerMessage(message: String) { | ||
self.controllerMessage = message | ||
} | ||
|
||
func addAction(_ actionType: AlertActionType, action: ((UIAlertAction) -> Void)? = nil) { | ||
let action = UIAlertAction(title: actionType.title, style: actionType.style, handler: action) | ||
alertActions.append(action) | ||
} | ||
|
||
func makeAlertController() -> UIAlertController { | ||
alertController.title = controllerTitle | ||
alertController.message = controllerMessage | ||
alertActions.forEach { alertController.addAction($0) } | ||
|
||
return alertController | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alertConfiguration 같은 struct 를 받아서,
파라미터로 해당 config(액션을 포함하거나 분리해서)를 받은 뒤,
받은 액션과 config들로 알럿을 띄운다면
AlertBuilder가 struct가 되고, 인터페이스도 하나로 통일될 거 같은데
어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested Type으로 Configuration 타입을 생성하여 builder 타입변환해주었습니다! 또 각 설정 메서드에 self를 반환하도록 하여 .으로 메서드가 연결될 수 있도록 구현해 보았습니다.
// Created by Max on 2023/09/26. | ||
// | ||
|
||
final class KeywordArgument { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 친구도 struct여도 되지 않나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞네요😂!! 변경하였습니다...!
protocol ViewModelType { | ||
associatedtype ViewModelError | ||
|
||
var error: Observable<ViewModelError?> { get set } | ||
|
||
func handle(error: Error) | ||
func setError(_ error: ViewModelError) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰 모델의 input과 output을 나눠서 정의해보는건 어떨까요?
뷰모델의 설계는 지금 워낙 제각각이긴 한데,
저는 Redux패턴이 가장 좋은거 같아요
대표적으로 TCA, ReactorKit 등이 있구요
input/output까지만 명시되는 뷰모델,
reduce까지해서 상태기반으로 정의되는 뷰모델
은 다르니까 고런 부분도 공부해보시면 좋을 듯 싶습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input/output 구분에서 정말 오랜 시간 고민을 하게 되었습니다 😭
다양한 패턴을 들여다보다 보니 실질적인 구현에 있어서 헷갈리는 부분이 많아
결과적으로 kickstarter의 Protocol 활용한 input/output 구분 방식을 채택하였는데 잘 정리가 되었는지 모르겠네요...
앞으로 말씀주신 부분도 참고하여 열심히 공부해보도록 하겠습니다!!
} | ||
|
||
override func viewWillAppear(_ animated: Bool) { | ||
viewModel.fetchAllData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰는 뷰모델에서 어떤 역할을 하는지까지는 몰라도 될 것 같아요
뷰는 어떤 액션이 뷰에서 일어났다 까지만 정의해주고
그 액션이 일어났을때 -> 어떤 일을 할거다 를 뷰모델에서 정의해주세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 제가 완전히 잘못 이해하고 있었던 것 같아서, 하비 말씀 듣고 나서야 조금 방향이 잡힌 것 같습니다😭
뷰모델이 뷰의 행위에 맞춰서 호흡을 같이할 수 있도록 메서드 형태를 전체적으로 수정하였습니다.
이 과정에서 UseCase라는 이름으로 ViewModel 내부 로직 일부가 분리되었는데(ToDo 데이터에 대한 공통 전처리로 뷰모델마다 반복되는 부분) 사실상 이게 UseCase가 맞는가...라는 고민은 들지만 달리 이름을 붙이기가 애매하여😭 일단 그렇게 네이밍을 하였습니다.
let alertBuilder = AlertBuilder(prefferedStyle: .alert) | ||
alertBuilder.setControllerTitle(title: error.alertTitle) | ||
alertBuilder.setControllerMessage(message: error.alertMessage) | ||
alertBuilder.addAction(.confirm) | ||
let alertController = alertBuilder.makeAlertController() | ||
present(alertController, animated: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 코멘트 단 대로
let alertBuilder = AlertBuilder.build(title, message, action...) 하나로도 가능해보여요
override func prepareForReuse() { | ||
titleLabel.textColor = .black | ||
bodyLabel.textColor = .black | ||
dateLabel.textColor = .black | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text도 초기화해야하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 이 부분을 놓쳤네요! 추가했습니다!
안녕하세요 하비(@havilog)! 🔹 AlertBuilder 타입 변경코멘트주신 내용을 토대로 Configuration 타입을 나누어 Builder는 struct가 될 수 있도록 하였습니다. 또한 값을 세팅하는 메서드마다 return self를 추가하여 .으로 모든 메서드가 연결될 수 있도록 변경하였습니다. 🔹 ViewModel을 Input/Output으로 구분POP를 활용하여 각 뷰모델의 Input에서 진행할 내용과 Output에서 전달받을 데이터를 구분하였습니다. 이를 통해 View에서는 Input/Output으로 지정된 내용으로만 접근할 수 있도록 제한하였습니다. 🔹 View의 상태/행위에 맞춰 ViewModel을 구성View에서는 ViewModel에서 어떤 것을 하는지 몰라도 된다는 말씀을 듣고 고민한 끝에, ViewModel의 구성을 View의 상태나 행위에 맞춰 진행할 수 있도록 변경하였습니다. 이에 따라 viewDidLoad에서 호출되는 내용, addChildren에서 호출되는 내용 등으로 메서드를 구분하였습니다. 🔹 ViewModel의 데이터 전처리 로직 분리ViewModel들이 모두 ToDo 엔티티를 활용하다보니 동일한 전처리가 들어가야 하는 부분들이 계속 반복되었습니다. 이 부분을 매번 delegate로 처리하는 것도 애매하다고 생각되어, delegate에서는 반드시 연결되어야 하는 부분(예: status가 바뀌었을 때 예전 테이블뷰와 새로운 테이블뷰가 모두 변경사항을 적용할 수 있도록 하기)에만 사용하고 나머지 로직은 UseCase로 분리하였습니다. 🔹 status 변경용 PopOverView 추가프로젝트 진행은 멈추라고 하셔서 이 부분은 사실 작업하지 않으려 하였는데... status 변경시 두 가지 테이블뷰에 적용이 되는지 여부를 확인하고 싶어서 추가 진행하였습니다. 아직도 많이 부족하지만, 하비 덕분에 많은 공부를 할 수 있었습니다! |
안녕하세요 하비(@havilog)!
프로젝트 매니저 STEP 2의 첫번째 PR을 올려드립니다...!
2-1번째인 이번 스텝은 테이블뷰에 데이터 뿌리기까지만 먼저 진행해보라는 하비의 말씀을 참고하여,
첫번째 뷰컨트롤러 화면에서 진행할 수 있는 내용까지 쪼개어 진행하였습니다.
(데이터 바인딩하여 화면에 뿌리기 & 스와이프로 삭제)
자세한 내용은 아래 정리하겠습니다!
고민했던 점
🔹 CoreDataManager
이번 프로젝트에서 로컬DB로
CoreData
를 사용하기로 정했기 때문에, 이번 스텝에서는CoreData
엔티티를 생성하고 관련 CRUD 로직을 처리할 수 있는CoreDataManager
타입을 만들었습니다. (CoreDataManager
는 다양한 엔티티와 함께 활용 가능하도록 제네릭 타입으로 구현하였습니다.)그런데 STEP 2 요구사항 중에 디스크 저장 없이 메모리에만 저장하는 형식으로 구현하라는 부분이 있어,
saveContext
에서 실제save()
를 호출하는 부분을 주석처리해 두었습니다.🔹 DB 엔티티 구성
TODO
/DOING
/DONE
을 각각의 엔티티로 나눌지, 아니면status
attribute를 주어서NSPredicate
를 통해 필터링해 구분할지를 고민하였습니다. 각 할일의 상태가 변할 때마다 데이터를 지우고 새로 쓰는 것보다는 상태값만을 수정하는 편이 데이터 유실 등의 위험이 적다고 판단되어 후자를 선택하였습니다.🔹 MVVM 구현 방식
UIKit
과MVVM
관련 자료나 레포를 찾아보니Clean Architencture
를 적용하거나Combine
과 함께 사용한 케이스가 많았습니다. 하지만 이번 단계에서는Combine
이나RxSwift
까지 생각하지 않고 진행해보라는 조언을 주셨었기에 일부러 해당 방식은 배제를 하고 진행해보려 했습니다. 결과적으로Observable
타입을 만들어 바인딩해주는 방식으로 진행하였습니다.Observable
타입은 위와 같이 생성하였습니다. 이처럼 구현하고bind
메서드를 통해listner
에escaping
메서드를 지정해 주면, 해당Observable
타입으로 감싼value
값이 변경이 될 때마다listner
메서드를 실행하게 됩니다. 이를 통해 구현부에서 값이 변할 때마다 특정 행위를 할 수 있도록 지정할 수 있습니다.ViewModel
의 경우 아래와 같이 프로토콜을 생성하여 구현하였습니다.fetch
/create
/update
/delete
는 데이터를 알맞게 변형하여CoreDataManager
에 지정된 CRUD 메서드를 호출합니다.데이터를 성공적으로 불러왔을 경우는
dataList
의 값을 바꿔주고(이때dataList
에 바인딩된listner
를 호출하여 UI를 그림), 중간에 에러가 발생한 경우는errorMessage
를 변경합니다(이때errorMessage
에 바인딩된listner
를 호출하여alert
생성).🔹 AlertBuilder
Alert
생성시 반복적인 작업을 조금이나마 줄이고 정리하고자AlertBuilder
를 생성하였습니다. 자주 사용되는action
은AlertActionType
으로 정리하였습니다.🔹 TableView와 헤더를 담은 UIView 생성
이번 프로젝트에서는 하나의
View Controller
안에 세 개의Table View
가 들어갑니다. 각Table View
는 헤더 타이틀과 롱프레스 액션 선택지를 제외하면 동일한 모양을 하고 있습니다. 때문에 저는Table View
와 헤더를 하나의View
로 분리하여 반복적으로 활용하면 좋을 것 같다는 생각을 하였습니다. 그래서ToDoListView
를 생성하였고, 해당 뷰에서 반복적인 테이블뷰 로직들을 처리하고 있습니다.(날짜가 지난 할일은 빨간색으로 바꾸는 로직, 왼쪽으로 스와이퍼하여 삭제하는 로직 포함)🔸 (아직 고민중인 점) View와 ViewModel 연결
각 테이블뷰마다 ViewModel을 가져야 할지, 아니면 ViewController에만 ViewModel을 연결하고 각각의 테이블뷰에는 해당 정보만을 뿌려줄지에 대해 고민을 하였습니다.
하지만 롱프레스 액션을 구현할 경우 ToDo에서 Doing으로, 또는 Done으로 데이터가 이동해야 하는데, 이때 각 뷰모델의 dataList를 연동할 방법을 찾기가 어려웠습니다. 때문에 View Controller만 ViewModel과 엮어주고, 해당 데이터에 변동이 생기면 세 가지 테이블뷰를 모두 reload하도록 구현해 두었습니다.
이 부분은 아직도 고민이 많이 되는 부분이라서 자신이 없습니다😭 조언 주시면 감사하겠습니다!!