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

[team-09][Lollo, ZG] 데이터 받아 테이블 뷰 표시하기 #14

Merged
merged 45 commits into from
Apr 26, 2021

Conversation

Settpark
Copy link

작업 내용

  1. MVVM의 구조로 설계하도록 노력했습니다.
  2. Network의 내부 함수를 한가지 기능만 할 수 있게 독립적으로 나누려고 노력했습니다.
  3. MVVM의 구조에서 계층 간의 의존 관계를 끊으려고 하였습니다.
  4. Mock API를 통하여 메인화면의 일부 데이터를 받아와서 테스트 하였습니다.

학습 키워드

  1. Network + Combine
  2. MVVM
  3. Clean Architecture

고민 & 질문 사항

  1. UseCase를 다루는 형태를 고민했습니다. UseCase를 알고 있어야 하는 대상의 구분, UseCase의 역할 등을 고민하였고
    현재 UseCase가 잘 쓰이고 있는 지에 대한 고민이 가장 컸습니다. -> 완전히 해결하지 못한 상태로 남아있습니다.
  2. 현재 ViewModel에 sideDishesCollection의 구체 타입을 알고 있습니다. 구조상 뷰모델에서 도메인의 entities를 알고 있는 것에 대한 고민이 있는 데, 이 것을 단지 프로토콜로 전환한다고 해서 의존성이 끊어지는 지에 대한 의문이 남아 현재 진행하지 않고 구체 타입을 알고 있습니다. 프로토콜로 개선하면 이 문제를 해결할 수 있을까요??
  3. Network 모델 설계 시 재사용성을 고민하였습니다. Network 객체의 url을 외부에서 지정해줄 수 있도록 구현하고, Request 생성 객체 메소드에서 Enum 타입의 RequestType과 EndPoint를 넘겨줄 수 있도록 했습니다. EndPoint의 경우 hash 값 등 반찬 cell 선택에 따른 값 설정이 필요할 수 있어서 추후 API 형식에 따른 타입 변경이 필요해보입니다.
  4. Repository의 역할을 구분하는 기준이 명확하지 않아 현재의 프로젝트에서 repository의 영역을 구현하지 못했습니다. 혹시 현재 프로젝트에서 어떠한 방식으로 repository를 구현해야 하는지, 방향성에 대해서 질문 드리고 싶습니다.

고정완 (Can) and others added 30 commits April 19, 2021 14:55
TableView와 기본 뼈대가 되는 Cell을 Storyboard에 추가
label 업데이트 함수 작성. 후에 수정 필요
테이블 뷰와 DataSource 연결
테이블 뷰와 delegate 연결 커스텀 헤더 지정
Feature/메인 화면 UI 구성 (#8)
버튼 및 취소선 추가구성 필요
- SideDish 및 Detail 구현
- 구조에 대한 논의 필요
- SideDish 단일 리스트를 관리하는 모델 생성
- 추후 의존성 약화 위해 프로토콜로 인터페이스 분리 필요
headerView의 백그라운드색 지정, init함수 수정
- SideDishes들의 리스트를 관리하는 모델 생성
- recentUpdate를 통해 데이터 버전이 관리하도록 구성
- Category enum 생성하여 카테고리 설정 안정화
- SideDishFindable 인터페이스 설정하여 의존성 개선
- SideDish 및 Detail 구현
- 구조에 대한 논의 필요
- SideDish 단일 리스트를 관리하는 모델 생성
- 추후 의존성 약화 위해 프로토콜로 인터페이스 분리 필요
- SideDishes들의 리스트를 관리하는 모델 생성
- recentUpdate를 통해 데이터 버전이 관리하도록 구성
- Category enum 생성하여 카테고리 설정 안정화
- SideDishFindable 인터페이스 설정하여 의존성 개선
- 작업 단위 별로 객체를 분리하여 구현
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.

안녕하세요. 반갑습니다. Napster라고 합니다.
부족하지만 저도 열심히 한번 리뷰해볼께요!!

살펴보니 여러명이서 작성한 코드인게 눈에 잘 보이더라구요. 이렇게 잘 보인다는건 코드 컨벤션이 전혀 없다는 뜻이고, 다른사람이 읽을때 매우 힘든 코드라는 점입니다. 팀을 이루고 함께 프로젝트를 만들어가면 여러사람이 봐도 보기 좋아야겠죠?

Swift Lint를 해당 프로젝트에 적용해보세요. 최소한의 컨벤션은 지킬 수 있을꺼에요.
더 나아가서 현재 팀원들과 함께 Coding Style를 만들어서 지켜보는 건 어떨까요? 나중에 회사에 가면 회사마다 코딩 스타일이 있고 거기에 맞게 작성을 해야합니다. 만약 없다면 직접 만들어야겠죠?

질문에 대한 답변

Q.

  1. UseCase를 다루는 형태를 고민했습니다. UseCase를 알고 있어야 하는 대상의 구분, UseCase의 역할 등을 고민하였고
    현재 UseCase가 잘 쓰이고 있는 지에 대한 고민이 가장 컸습니다. -> 완전히 해결하지 못한 상태로 남아있습니다.

A.

UseCase의 사용은 적절해보입니다. 다만 Entity와 UseCase가 강한 결합도가 있는게 아쉽습니다.
현재 func manufactureForMainView() -> AnyPublisher<SideDishesCollection, Error>
는 SideDishesCollection를 서버에서 받은 그대로 사용하고 있는데, SideDishes는 언제든 바뀔 수 있습니다. SideDishFindable 만들어서 결합도가 느슨해진줄 알았는데, SideDishesCollection의 body를 직접 ViewModel에서 사용하다보니 그렇게 되지 않게 되었습니다.
해당 앱의 Main에서 사용할때 느스한 결합도를 가진 적절한 Entities로 바꿔보세요.
지금은 SideDishFindable로 의존성을 느슨하게 해주었지만 sideDishesCollection에서 구체화된 객체를 선언해서 쓰고있으니 해당
이점이 사라져 버렸습니다.

Q.

  1. 현재 ViewModel에 sideDishesCollection의 구체 타입을 알고 있습니다. 구조상 뷰모델에서 도메인의 entities를 알고 있는 것에 대한 고민이 있는 데, 이 것을 단지 프로토콜로 전환한다고 해서 의존성이 끊어지는 지에 대한 의문이 남아 현재 진행하지 않고 구체 타입을 알고 있습니다. 프로토콜로 개선하면 이 문제를 해결할 수 있을까요??

A.

1번에 대한 답이 어느정도 여기에도 포함되어 보이네요.
객체들간의 의존성을 완전히 끊을 수는 없습니다. 객체지향프로그래밍은 객체들간의 협력을 통해 이뤄지고 있으니까요. 의존은 필연적으로 발생하게 됩니다. 다만 어느 정도의 결합도를 가졌느나가 중요합니다.
객체들간의 의존을 프로토콜로 개선하면 결합도를 느슨하게 할 수 있습니다. 😙

Q.

  1. Network 모델 설계 시 재사용성을 고민하였습니다. Network 객체의 url을 외부에서 지정해줄 수 있도록 구현하고, Request 생성 객체 메소드에서 Enum 타입의 RequestType과 EndPoint를 넘겨줄 수 있도록 했습니다. EndPoint의 경우 hash 값 등 반찬 cell 선택에 따른 값 설정이 필요할 수 있어서 추후 API 형식에 따른 타입 변경이 필요해보입니다.

A.

Moya를 한번 참고해보세요. :)
https://moya.github.io

Q.

  1. Repository의 역할을 구분하는 기준이 명확하지 않아 현재의 프로젝트에서 repository의 영역을 구현하지 못했습니다. 혹시 현재 프로젝트에서 어떠한 방식으로 repository를 구현해야 하는지, 방향성에 대해서 질문 드리고 싶습니다.

A.

현재 Use Case인 TurnonAppUsecase에 NetworkManager만 있는데 외부에서 Repository를 주입해줘야겠지요?
그렇게되면 NetworkManager의 구체를 사용하는게 아닌 Repository를 사용하게 될것입니다.

Repository를 사용하는 이유는 무엇일까요? (한번 생각해보세요)

앱에서 사용하는 데이터가 API, Local Storage 등등 을 구분하지 않고 UserCase는 Repository에 메서드들만 사용하여 해당 데이터를 얻기만 하면 됩니다. 그러면 Repository가 NetworkManager를 의존하는 구조로 바뀌어야 할 것 같습니다.

}

func tableView(_ tableView: UITableView, heightForHeaderInSection section: Int) -> CGFloat {
return 56 // headerView의 height(32) + figma에서 지정해 준 헤더와 셀 간의 간격!

Choose a reason for hiding this comment

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

상수값으로 따로 선언해서 관리하는게 좋아보입니다.
직접적인 값의 사용은 좋지 않습니다 🥺

@eeeesong
Copy link

안녕하세요 Napster! 저는 Lollo 입니다.
우선 꼼꼼하게 리뷰 해주셔서 정말 감사드립니다☺️
그리고 저희의 고민과 질문에 대해서도 너무 상세하게 잘 답변을 해주셔서 큰 도움이 될 것 같습니다!

저의 경우 이번 프로젝트에서는 Network에서 뿐만아니라 Core Data와 같은 Storage를 사용하게 되어서
Use Case, Repository 등 여러 계층을 구조적으로 잘 관리하기 위해 자연스럽게 Clean Architecture에 눈이 갔던 것 같습니다 :)

오늘 리뷰해주신 부분들 내일까지 ZG와 함께 한번 더 꼼꼼하게 확인해서 수정을 거친 뒤
다시 이곳에 push 해보도록 하겠습니다.

정말 감사합니다!

@pay-napster-x
Copy link

화이팅 하시죠!!
저도 최대한 열심히 리뷰해볼께요! 부족하지만 감사합니다. 😄

eeeesong and others added 5 commits April 22, 2021 23:58
- CustomToaster: 각 토스트의 생성 메소드 분리하여 UI 정교화
- Pods-ToastView: Bubble의 Alignment를 설정할 수 있도록 변경
dquote>
dquote> class -> struct로 변경
dquote> 변수 var에서 let으로 변경
dquote> bind private 설정, Cold Observalbe 문제 해결
dquote> print문 제거 사용자에게 alertMessage 뜨도록 수정
dquote>
dquote> colleciton에 해당하는 protocol선언 (임시)
dquote> TablecustomHeaderView 선언
- NetworkManager: Mock 객체 초기화 시에도 URL 외부 주입, 쓰지 않는 URLSession 삭제
- SideDish: CodingKey 추가
@eeeesong
Copy link

안녕하세요 Napster, 좋은 아침입니다☀️
어제 주신 피드백 바탕으로 수정을 마쳤습니다 :)
다음 목록을 수정했고, 코멘트 달아주신 부분에도 추가 코멘트 달아두었습니다!

  • SideDish: Class -> Struct, var -> let
  • NetworkManager: Mock 객체 초기화 시에도 url 외부 주입, URLSession 부분 수정
  • MainViewController: bind를 private하게, Cold Observable 문제
  • MenuCellViewModel: print문 제거 -> 사용자에게 문제 상황 알리기
  • MainTableViewDataSource: cell 이름 static 상수로 변경
  • MainTableViewDataSource: 강제 언래핑 제거 ➡️ 실제 API를 적용하는 과정에서 수정해보겠습니다
  • MainTableViewDelegate: headerView 분리하여 생성, 높이 값 상수로 저장

문제가 없으시다면 머지 부탁드리겠습니다 :) 감사합니다!

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.

몇가지만 조금 수정하면 될 것 같습니다.


}

func configureAlertMessage(completion : @escaping ((String)->())){

Choose a reason for hiding this comment

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

이왕 Combine을 쓰고있으면 completion handler말고 publisher로 해결해보는건 어떨까요?

Choose a reason for hiding this comment

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

error alert 처리도 publisher로 바꾸었습니다 :)

}

private func showAlert(message : String){
DispatchQueue.main.async {

Choose a reason for hiding this comment

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

캡쳐리스트는 꼭 필요합니다.
강한참조가 발생할 수 있거든요 😆

@eeeesong
Copy link

eeeesong commented Apr 25, 2021

안녕하세요 Napster :) 수정이 조금 늦어졌습니다. 더불어 일요일에 코멘트를 남기게 되어 죄송하다는 말씀 드립니다🥲

mainMenuViewModel.$errorMessage
             .receive(on: DispatchQueue.main)
             .sink(receiveCompletion: { _ in}, receiveValue:{ [weak self] error in self?.showAlert(message: error) })
             .store(in: &self.subscription)

위와 같이 수정을 했는데, 말씀하신 강한 참조 부분을 제대로 해결했는지 모르겠습니다ㅠㅠ
[weak self]로 작성한 부분으로 캡처 부분을 해결했다고 생각했는데, 괜찮으시다면 맞게 한 것인지 한번 확인 부탁드립니다!

jjunyjjuny pushed a commit that referenced this pull request Apr 26, 2021
[0421] Refactor BestList, Badge, Card, Modal
@pay-napster-x
Copy link

네 캡쳐 리스트를 통해 강한 참조 발생을 방지할 수 있습니다.
꼭! 어떨때, 어떻게 써야하는지 숙지하고 계셔야합니다.

capture list

@pay-napster-x pay-napster-x merged commit 5f10f8b into codesquad-members-2021:ghojeong Apr 26, 2021
@eeeesong
Copy link

꼭! 어떨때, 어떻게 써야하는지 숙지하고 계셔야합니다.

넵 명심하고 익히겠습니다!
감사합니다 :)

ksundong pushed a commit that referenced this pull request Apr 28, 2021
ksundong pushed a commit that referenced this pull request Apr 28, 2021
[BE] Model 설계 

closes #4
sallyjellyy pushed a commit that referenced this pull request Apr 29, 2021
메인 카테고리 Mockup API
wheejuni pushed a commit that referenced this pull request Apr 29, 2021
[BE] 스프링 개발환경 세팅
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

4 participants