일기장 [STEP 2] Idinaloq, Yetti #135
Conversation
AppDelegate, SceneDelegate 파일 swiftLint 제외
ViewController.swift -> DiaryListViewController.swift 네이밍 변경
화면에 데이터 받아와서 표시, 수정가능하도록 추가
Diary Entity가 UUID를 가지도록 model data 추가
There was a problem hiding this comment.
- 첫 번째 줄을 제목으로 지정하기
사실 이 부분은,, 제목 뷰와 본문 뷰가 나뉘도록 기획, 디자인을 변경해서 뷰를 쪼개버리면 해결될 거 같아요
모든 요구사항에 기능을 맞추기보다는 어떻게 해야 사용자 경험이 제일 좋을까를 고민해보시는 것도 추천드립니다~
- collectionView 그려주는 것과 액션 분리하기
코멘트에도 달았는데 이미 고민하신 부분이군요..!
저라면 viewDidLoad() 시점에 해줄거 같아요.
collectionView가 lazy여도 괜찮다 라는 기준이 있으면,
함수로 최대한 쪼갤 거 같구요
- 코어데이터의 Create 메서드
저라면.. 그냥 let id: UUID 같은 값을 가지고 객체 생성 시점에 넣어줄거 같아요
- 코어데이터의 SaveContext 메서드
4.1
-> struct와 class의 차이, 코어데이터 내부에서 컨테이너가 어떻게 객체를 핸들링하는지
공부해보시면 좋을 것 같습니다.
4.2
뷰와 로직은 분리되는것이 좋을 거 같아요
뷰 -> 뷰의 기능을 핸들링하는 객체(뷰모델) -> 코어데이터를 담당하는 객체 -> 코어데이터에 접근해서 값 수정
| import UIKit | ||
| import CoreData | ||
|
|
||
| final class CoreDataManager { |
There was a problem hiding this comment.
- 이 친구는 반드시 싱글톤이어야 하는지?
- 싱글톤일 때 장점과 단점
- 에러 핸들링을 print, fatalError를 찍으면 사용자는 일기의 작성이 실패했는지를 어떻게 알 수 있을지
- 뷰가 이 의존성을 가지고 있을 때 테스터빌리티에 미치는 영향은?
- 외부에서 접근하는 함수를 protocol화 하면 어떤 장단점이 있을지?
- UIKit의 import는 필요할지?
- Diary의 id값은 optional이 아니어도 될 것 같아요
등등 생각해보시면 좋을 거 같아요
또한 8기 분들의 코드가 전반적으로 비슷한 거 같은데,
이 CoreDataManager 라는 객체의 역할과 구현은 직접 생각하신 부분일까요??
There was a problem hiding this comment.
-
CoreDataManager자체는 데이터를 저장하지 않고 처리해주는 책임을 가지고 있기 때문에 반드시 싱글톤일 필요는 없을 것 같습니다.
-
장점- 인스턴스 생성을 한번만 함으로써 데이터를 공유할 수 있고 일관된 접근으로 수정도 가능합니다.
단점 - 다중 스레드 환경이라면 동시에 데이터에 접근하는 경우 동기화 문제가 발생할 수도 있고 테스트도 어렵다는 단점이 있습니다. -
현재 상태에선 사용자가 알 수 없을 것 같습니다..! 저희가 생각한 방법으로는 Alert을 만들어 일기의 작성이 실패했을 때 알려주는 방법이 있을 것 같습니다.
-
각각의 테스트마다 객체가 값을 변경하게 되면 다른 테스트에도 영향을 미칠 수도 있기 때문에 유닛 테스트에 어려움이 있을 수도 있습니다.
-
프로토콜로 정의하게 되면 직접 구현하는 코드와 분리할 수 있고 코드의 모듈화와 재사용성을 높일 수 있습니다.
-
필요하지 않을 것 같습니다.
-
UUID를 외부에서 주입받지 않도록 create내부에서 생성해줌으로써 optional을 떼 줄 수 있습니다. 등등 생각해보시면 좋을 거 같아요
- 제가 8기 캠퍼이신 릴라에게 멘토링을 받는 시간이 있어 코어데이터에 대해 많이 물어보고 질문하여 어느정도 의견이 반영된 부분이 존재하긴하지만 메서드 구현자체는 팀원끼리 했었습니다!
There was a problem hiding this comment.
- 싱글톤이 무조건 나쁜건 아니지만, 신입 때 편하다는 이유로 사용하는 경우가 많아서, 반드시 근거를 가지고 사용해주세요!
- 장점과 단점 모두 actor에 대해서 공부해보시면 좋을 듯 합니다. -> async await랑 같이 공부해야할거라 어려우실 거에요.
- 에러를 면으로 표시할지, 알럿으로 할지, 토스트로 할지, 무시할지 등은 정책적인 부분인데요, 해당 사항들에 대해 어떻게 처리할 지에 대한 근거는 생각해보시는 습관이 좋을거 같아요 ㅎㅎ print나 return은 안좋습니다!
- solid에서 d에 대한 부분과, 의존성 주입(dependency injection)을 활용해 mock객체를 만들고 뷰와 로직의 분리를 공부해보시면 좋을 거 같습니다.
- 👍
- 👍
- 👍
다른 사람의 코드를 보고 작성하는 것은 중요하고 좋은 일인데요,
잘 짜여진 코드를 분류하고 / 해당 코드가 왜 그렇게 작성됐는지 근거를 생각하고 작성하시면 좋을 듯 합니다~
| label.translatesAutoresizingMaskIntoConstraints = false | ||
| label.numberOfLines = 1 | ||
| label.font = UIFont.systemFont(ofSize: 12) | ||
| label.setContentHuggingPriority(.init(300), for: .horizontal) |
There was a problem hiding this comment.
extension UILayoutPriority에 있는 .defaultLow과 같은 값을 사용하지 않고 커스텀하게 정의하신 이유가 있을까요?
There was a problem hiding this comment.
defaultLow와 defaultHigh를 사용해 보았는데 previewLabel의 내용이 짧은 경우 dateLabel이 레이아웃을 제대로 잡지못하는 이슈가 있었습니다.
그래서 고정적인 값을 설정해주게 되었고 현재 상태에선 결과적으로 레이아웃 이슈가 발생하지 않습니다.
There was a problem hiding this comment.
제가 뷰 쪽 코드는 잘 보지 않지만,
아마 resistence를 조정하거나,
다른 방식으로 뷰를 그린다면(스택뷰를 안쓴다거나, constraint를 다른곳에 건다거나) 해결될 문제일 가능성이 높아서,
좀 더 고민해보시면 좋을 듯 하네요!
(틀렸다는건 아니고 이게 최선일 수도 있어요)
| let title = diary.title | ||
| let alertController = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet) | ||
|
|
||
| let shareAction = UIAlertAction(title: "Share", style: .default) { _ in |
There was a problem hiding this comment.
만약 closure가 @escaping한다면 self를 참조할 때 어떤 부분을 유의해야하는지 설명해주시면 좋을 거 같습니다~
There was a problem hiding this comment.
escaping은 함수가 종료되어도 해당 함수의 scope를 벗어나 함수 종료 후에 실행되기 때문에 강한 순환 참조가 발생할 수 있습니다!
There was a problem hiding this comment.
순환 참조 이외에도 delay deallocation이 발생할 수도 있어서 앵간하면 weak 는 습관 잡아주세영
| } | ||
|
|
||
| extension DiaryDetailViewController: UITextViewDelegate { | ||
| func textViewDidEndEditing(_ textView: UITextView) { |
There was a problem hiding this comment.
- textView에서 text를 발라오는 부분
- text에 대한 비지니스를 처리하는 부분 -> 에서 subscript 예외처리도 고민해보시면 좋을 것 같습니다
- 비지니스 로직으로 처리된 결과값을 가지고 새 일기 객체를 만드는 부분
- 새로운 일기 객체를 저장 시도
- 성공 시 self의 일기를 변경 / 실패시 알럿
과 같이 분리해보아도 괜찮을거 같습니다~!
There was a problem hiding this comment.
모든 부분을 반영하진 못했지만 textViewDidEndEditing() 내부의 로직을 분리하는 작업을 해보았습니다.
private func splitText() -> (title: String, body: String)? {
guard let text = textView.text?.trimmingCharacters(in: .whitespacesAndNewlines),
!text.isEmpty else {
return nil
}
let lines = text.components(separatedBy: "\n")
let title = lines.first ?? "일기 제목"
let body = lines.dropFirst().joined(separator: "\n") + "\n"
return (title: title, body: body)
}
private func getDiaryContents() {
let text = splitText()
diary.title = text?.title
diary.body = text?.body
}
func textViewDidEndEditing(_ textView: UITextView) {
getDiaryContents()
CoreDataManager.shared.saveContext()
}받아온 텍스트를 title과 body로 분리하는 메서드와 분리한 title과 body를 diary에 넣어주는 메서드로 분리해주었고 textViewDidEndEditing()에서는 다이어리 내용만 가져올 수 있도록 해주었습니다.
하비가 말씀하신 서브스크립트 예외처리는 어떤 부분인지 감이 잡히지 않아 수정하지 못했고 5번의 알럿부분도 로직에서 크게 넣을만하지 않을 것 같아서 넣지 않았습니다.
혹시 저희가 하비의 의도와 다르게 이해한 바가 있다면 가르쳐주시면 감사하겠습니다!
There was a problem hiding this comment.
모든 함수, 객체 등에는 의존성이 있으면 확장성이 덜어지는데요,
getDiaryContents에서는 UITextView의 text를 파라미터로 넣어주고,
반환값으로 필요한 객체(여기선 Diary)를 반환해주면 될 것 같아요
| final class DiaryListViewController: UIViewController { | ||
| var diaries: [Diary] = [] | ||
|
|
||
| private lazy var collectionView: UICollectionView = { |
There was a problem hiding this comment.
- collectionView를 초기화해주는 부분이 너무 길어서, trailing을 넣어주는 부분을 따로 빼도 좋을 거 같아요
2. self를 참조하지 않는다면 load시점에 만들고 이후에 프로퍼티들을 초기화해줘도 괜찮지 않을 까 싶어요
3. delete에는 [weak] 가 잡혀있고, share에는 안잡혀있네요~!
4. delete traling action 실행되었다 와 실행되었을 때 어떤 로직을 처리할거다 가 분리되어있으면, 테스트하기도 좋고 더 가독성있는 코드가 될 것 같습니다.
There was a problem hiding this comment.
ee1e8cc
share에 [week self]가 빠진것을 확인을 제대로 못했었네요... 이 부분을 포함해서
delete trailing action같은 경우,
swipeAction(_ configuration: inout UICollectionLayoutListConfiguration) { ... }메서드로 분리만 일단 해 놓은 상태입니다.
말씀하신 부분들 중 1,2번의 경우는 감이 잡히질 않아 좀 더 고민해봐야 될 것 같습니다
There was a problem hiding this comment.
lazy var로 뷰가 초기화 되면, variable이기 때문에 의도치 않은 곳에서 새로 생성될 수 있는 낮은 가능성이 생겨서
let으로 할 수 있으면 하는게 좋다 정도 긴해요
| configureLayout() | ||
| } | ||
|
|
||
| override func viewDidAppear(_ animated: Bool) { |
There was a problem hiding this comment.
-
반드시 appear시점이 아닌, 로드시점에 일기를 핸들링하는 객체가 일기를 가져오게 수정해볼 수 있을 것 같습니다.
2. reloadData()는 모든 데이터를 전부 새로 그릴 때 사용하는데, 삭제, 추가 시점에 자연스러운 애니메이션과 함께 전체를 다시 그리지 않는 방법에대해서도 고민해보시면 좋을 것 같습니다.
3. 전반적으로 가드문을 많이 쓰신다고 느껴지는데, 이러면 의도치 않은 버그들이 많을 것 같습니다.3-1. 반드시 optional로 해야하는 아이들인지 생각해보고
3-2. 에러로 처리해야하는 상황일지 return시켜도 될지 더 깊은 고민이 필요해보입니다~!
There was a problem hiding this comment.
말씀하신 2번에서 삭제할 때의 애니메이션을 performBatchUpdates를 사용해서 적용해보았습니다. 해당 문서에서 Related Documentation 의 메서드들을 적용하려고 했는데, insertItems의 경우 indexPath를 받아올 방법이 떠오르지 않네요..
말씀하신 부분들 다음 프로젝트를 진행할 때 참고하도록 하겠습니다!!
There was a problem hiding this comment.
collection view나 tableview를 그릴 때 부분적 업데이트를 하는 것은 매우 중요해요
원하는 아이템만 추가/삭제 하는 법에 대해 고민해보시고,
diffable datasource로 그리는 법도 공부해보시면 좋을 듯 싶네요!
안녕하세요 하비! @havilog
이디나로크, 예티 팀입니다!
저희에겐 꽤나 어렵고 험난한 CoreData네요 ㅠㅠ
아직 많이 부족하고 아쉬운 점이 많지만 좋은 리뷰 부탁드립니다!
고민했던점
1.
reloadDatacedb035
viewWillAppear()에 호출하는 메서드를 넣어주고reloadData()로 뷰를 그려주려고 했습니다 하지만 이렇게 구현하게 되면 새로운 Diary를 만들었을 때 리스트에서 보이지 않는 문제가 있었습니다. 저희가 파악한 문제점은 View가 보여지기 전인viewWillAppear()에서reloadData()로 뷰를 그려주려고하는 뷰의 생명주기의 문제였습니다. 최종적으로 뷰가 생성된 후 그려줄 수 있도록viewDidAppear()메서드 내부에서reloadData()를 호출해주었습니다.2.
비슷한 기능의 두개의 ViewController 합치기300176c
DiaryListViewController, 작성된 일기 내용을 보여주는DiaryDetailViewController, 일기장을 새로 만드는NewDiaryViewController총 세 개가 있었습니다.createNewDiaryButtonTapped()메서드에서 버튼을 눌렀을 때 생성이 되고,viewDidAppear가 호출이 되었을 때 만약 값이 비어있다면 삭제 하는 방식으로 작성하였습니다.3.
CoreData 기능 분리CoreData를 활용하는 파일을 만들어CoreData의 기능을 최대한 분리해주기 위해서CoreDataManager라는 파일을 따로 만들어서CoreData를 관리해주었습니다. 하지만 CRUD를 구현하다보니CoreData를 사용하는 VC에서는 모두CoreData를 import해주고 fetch하기위해fetchRequest프로퍼티도 가지고 있어야하는 문제점이 있었습니다. 이 문제점을 해결하기 위해 저희는NSFetchRequest<Diary>타입을 매개변수로 받아와야하는fetch()메서드는CoreData내부에서fetchSingleDiary()와fetchAllDiaries()메서드에서만 호출할 수 있게 해주고 각각의 메서드를 필요한 곳에서 호출할 수 있도록 해주었습니다.조언을 얻고 싶은 점
1.
첫 번째 줄을 제목으로 지정하기cedb035
title에, 넘어간 후 부터는body에 저장되는 코드를 작성하였으나, 문자열 인덱싱 과정에서 잘리는 부분과 입력이 한 박자 늦는 문제점이 있었습니다.textViewDidChangeSelection(_:)메서드가 실행될 때 입력했던 시점보다 한 번씩 밀려서 실행되는 것 같습니다. 해당 메서드에서 계속해서 타이틀 값의 조건을 비교하는 방법 이외에textViewDidEndEditing()메서드에서 해결할 수 있을 것 같다고 생각하지만 생각처럼 잘 되지가 않아서 해당부분은 어떻게 해야될 지 여쭤보고 싶습니다.2.
collectionView 그려주는 것과 액션 분리하기DiaryListVC의collectionView를 보게 되면 view를 만들어주는 것과 view가 수행하는 행위등의 코드가 함께 있어collectionView가 너무 비대해졌습니다. swipe액션을 수행하는 부분을 메서드로 빼야하는가도 고민해봤는데 view를 그려주는 곳에서 메서드의 호출이 적절한가에 대해서도 고민했었습니다. 하비가 보실 때는 어떤 방법이 나은지 다른 좋은 방법은 없을지 고민되어 질문드립니다.3.
코어데이터의 Create 메서드cedb035
CoreDataManager의create(diary uuid: UUID)메서드에서UUID를 전달받고 있습니다.Diary가 코어데이터에 저장될 때[Diary]와 같은 배열로 저장되고 있기 때문입니다.fetchSingleDiary(by uuid: UUID)메서드에서 NSPredicate를 통해 원하는 인덱스에 접근하도록 다음과 같이UUID를 사용해야 하지만create에서UUID값을 직접 만든다면DiaryListViewController의createNewDiaryButtonTapped메서드에서 일기를 생성을 하고 생성된 일기의 데이터를 다음 뷰로 넘겨야 되는데,UUID값을 알지 못해 넘겨줄 수 없다고 생각합니다. 또한create메서드에서 코어데이터를 생성할 때UUID를 반환할 수 있다고 생각이 되지만, 그러면create라는 이름과 맞지 않는다고 생각합니다.create()메서드에서UUID를 주입받는것 보다는 직접 만드는게 맞는 방법일까요? 아니면 지금과 같은 방식도 괜찮은걸까요?4.
코어데이터의 SaveContext 메서드cedb035
DiaryDetailVC에init으로Diary를 전달받고 있습니다. 전달 받은 값을textViewDidEndEditing()메서드에서 업데이트를 하고 있는데, 처음에는 단순하게DiaryDetailVC의diary프로퍼티를 업데이트 하려고 생각했습니다. 하지만diary프로퍼티가 아닌 기존에 존재하는 코어데이터의Diary자체에 값이 저장되는것을 확인했습니다.init과정에서 전달받은 전달인자diary가 값이 복사되는 것이 아닌 코어데이터의Diary에 접근하는 행위인지 궁금합니다.diary가 코어데이터에 저장된Diary라면 지금과 같은 방법으로textViewDidEndEditing()메서드에서saveContext()로 접근, 수정을 하고 저장을 해도 되는지 궁금합니다.